10

While trying to write a class that times the duration between calling it's constructor and destructor, I ran into what I think is a bug in clang. (Edit: it's not a bug; it's implementation defined copy elision)

The timer struct below keeps a pointer to the duration object that's passed in as reference and adds the duration of the scope to this.

#include <iostream>
#include <chrono>
struct timer {
    using clock      = std::chrono::high_resolution_clock;
    using time_point = clock::time_point;
    using duration   = clock::duration;
    duration* d_;
    time_point start_;
    timer(duration &d) : d_(&d), start_(clock::now()) {}
    ~timer(){
        auto duration = clock::now() - start_;
        *d_ += duration;
        std::cerr << "duration: " << duration.count() << std::endl;
    }
};

timer::duration f(){
    timer::duration d{};
    timer _(d);
    std::cerr << "some heavy calculation here" << std::endl;
    return d;
}

int main(){
    std::cout << "function: " << f().count() << std::endl;
}

When compiling this with clang 7.0.0, the output is:

some heavy calculation here
duration: 21642
function: 21642

while for g++ 8 the output is

some heavy calculation here
duration: 89747
function: 0

In this case I do like clangs behavior, but from what I've found elsewhere the return value is supposed to be copied before destructors are run.

Is this a bug with Clang? Or does this depend on (implementation defined?) return value optimization?

The behavior is the same independent of whether the duration d in timer is a pointer or a reference.

--

I do realise that the compiler inconsistency can be solved by changing f so that the scope of the timer ends before the return, but that's beside the point here.

timer::duration f(){
    timer::duration d{};
    {
        timer _(d);
        std::cerr << "some heavy calculation here" << std::endl;
    }
    return d;
}
Ragnar
  • 434
  • 7
  • 16
  • 1
    FYI, your naming conventions are making your code hard to read. Please try not to recycle the same identifiers for different things within the same scope. –  Feb 07 '19 at 01:56
  • 1
    I wonder if this might be related to NRVO... Which c++ standard version are you compiling against? –  Feb 07 '19 at 01:58
  • @Frank For g++ both c++11 and c++17 return 0 from the function, while clang returns nonzero for both c++11 and c++17. – Ragnar Feb 07 '19 at 02:01
  • 3
    Debugging the output of gcc, it does seem that gcc constructs a copy for the returned value before running the destructors. A copy of `d` gets constructed before the destructor runs. Examining the destructor's `this`, and `this` of what `f()` returns, they are different objects. I parse cppreference.com's entry on copy elision indicating that in this instance it is optional, and not required, and thus is implementation-defined. clang apparently elides the copy, while gcc does not; but I am not certain enough, of my interpretation, to put this down as an answer. – Sam Varshavchik Feb 07 '19 at 02:11
  • 2
    @SamVarshavchik I think you're right. [stmt.return](http://eel.is/c++draft/stmt.return#3) says the copy-construction of the return value is sequenced before destructors of local variables. The only way the clang output could happen is if the copy is elided and `d` and the return value of `f` are the same object. – Miles Budnek Feb 07 '19 at 02:16
  • @SamVarshavchik from that page: "When copy elision occurs, the implementation treats the source and target of the omitted copy/move (since C++11) operation as simply two different ways of referring to the same object ..." I suppose that might be the answer, although somewhat hidden. – Ragnar Feb 07 '19 at 02:23
  • I suggest the behaviour indicates no bug in either compiler. The difference is in whether a copy of `d` is elided, which seems be implementation defined (or unspecified?). It appears that cang elides a copy in returning from `f()` so, in modifying `d`, the destructor modifies the value that the caller sees. It appears g++ is not eliding the copy so, in modifying `d`, the destructor affects a different object from the one returned to the caller. As you've noted, placing the timer in an inner scope gives consistent results - because the destructor completes before the return statement – Peter Feb 07 '19 at 02:49
  • Answer to the related question whether [NRVO is mandatory in C++17](https://stackoverflow.com/questions/41466847/mandatory-copy-elision-gcc-5-4-1#41466926) – Arne Vogel Feb 07 '19 at 13:42

1 Answers1

7

Short answer: due to NRVO, the output of the program may be either 0 or the actual duration. Both are valid.

For background, see first:

Guideline:

  • Avoid destructors that modify return values.

For example, when we see the following pattern:

T f() {
    T ret;
    A a(ret);   // or similar
    return ret;
}

We need to ask ourselves: does A::~A() modify our return value somehow? If yes, then our program most likely has a bug.

For example:

  • A type that prints the return value on destruction is fine.
  • A type that computes the return value on destruction is not fine.
Acorn
  • 24,970
  • 5
  • 40
  • 69