LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 12139 - CLANG copy ctor with default arguments disregard passed-in arguments when invoked with an Rvalue source object
Summary: CLANG copy ctor with default arguments disregard passed-in arguments when inv...
Status: RESOLVED FIXED
Alias: None
Product: clang
Classification: Unclassified
Component: C++ (show other bugs)
Version: trunk
Hardware: Macintosh MacOS X
: P enhancement
Assignee: Unassigned Clang Bugs
URL:
Keywords: miscompilation
Depends on:
Blocks:
 
Reported: 2012-02-29 15:05 PST by Andrea Bigagli
Modified: 2012-10-05 20:37 PDT (History)
4 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrea Bigagli 2012-02-29 15:05:22 PST
Disclaimer: I had a hard time trying to squeeze out a meaningful Summary for this, hope this makes sense after all.

This problem caught me for real while using an open source library, where the effect was that an object copy constructed in this way didn't get the proper values from the passed in arguments.

I reproduced a reduced test-case herebelow:


CODE:

#include <iostream>
#include <cstdint>
#include <cstddef>
#include <cassert>

enum value_t 
{ 
     ORIGINAL     = 0
    ,DEFAULT_ARG  = 1
    ,PASSED_IN    = 2
    ,DEFAULT_CTOR = 3
};

struct A
{
    A () : val(DEFAULT_CTOR) {}
    A (A const &rhs, value_t a_val=DEFAULT_ARG) : val(a_val) {}

    A makeA()
    {
        A a;
        return a;
    }

    A makeStaticA()
    {
        static A a;
        return a;
    }

    value_t val;

};

int main(int argc, const char *argv[])
{
    {
        //Rvalue makeA testcase: clang fails (Expected PASSED_IN, Actual DEFAULT_CTOR), gcc succeeds (Expected PASSED_IN, Actual PASSED_IN)
        A original;
        original.val = ORIGINAL;

        A other(original.makeA(), PASSED_IN);

        std::cout << "Rvalue makeA:       " << (other.val == PASSED_IN) << " (Expected:" << PASSED_IN << ", Actual:" << other.val << ")" << std::endl;
        //assert (other.val == PASSED_IN); 
    }

    {
        //Rvalue makeStaticA testcase: clang fails (Expected PASSED_IN, Actual DEFAULT_ARG), gcc succeeds (Expected PASSED_IN, Actual PASSED_IN)
        A original;
        original.val = ORIGINAL;

        A other(original.makeStaticA(), PASSED_IN);

        std::cout << "Rvalue makeStaticA: " << (other.val == PASSED_IN) << " (Expected:" << PASSED_IN << ", Actual:" << other.val << ")" << std::endl;
        //assert (other.val == PASSED_IN);
    }


    {
        //Lvalue makeA testcase: clang succeeds, gcc succeeds
        A original;
        original.val = ORIGINAL;

        A const &cloned = original.makeA();
        A other(cloned, PASSED_IN);

        std::cout << "Lvalue makeA:       " << (other.val == PASSED_IN) << " (Expected:" << PASSED_IN << ", Actual:" << other.val << ")" << std::endl;
        //assert (other.val == PASSED_IN);
    }

    {
        //Lvalue makeStaticA testcase: clang succeeds, gcc succeeds
        A original;
        original.val = ORIGINAL;

        A const &cloned = original.makeStaticA();
        A other(cloned, PASSED_IN);

        std::cout << "Lvalue makeStaticA: " << (other.val == PASSED_IN) << " (Expected:" << PASSED_IN << ", Actual:" << other.val << ")" << std::endl;
        //assert (other.val == PASSED_IN);
    }

    return 0;
}

I think the code is well formed, but when compiled on OSX 10.7.3, with clang++ from trunk (r151190) or Apple's Xcode 4.3 release, the outcome is:

OUTCOME:
Rvalue makeA:       0 (Expected:2, Actual:3)
Rvalue makeStaticA: 0 (Expected:2, Actual:1)
Lvalue makeA:       1 (Expected:2, Actual:2)
Lvalue makeStaticA: 1 (Expected:2, Actual:2)

While, as stated in the comments, all cases pass with gcc-4.6.2.

Either this is expected behavior and I apologize for not understanding the standard, or it seems like something (related to maybe RVO?) causes the additional argument to CCTOR to be ignored when an Rvalue for the source object is passed to the CCTOR.
Comment 1 Andrea Bigagli 2012-03-01 03:04:02 PST
For completeness, and since I got a green flag from its author, I can point you to the specific project where I was hit by this: RCF - Remote Call Framework (http://www.deltavsoft.com/). The problem manifests while receiving packets, and the only way I could make it work when building with clang was to apply the following patch: 

Index: src/RCF/ConnectionOrientedClientTransport.cpp
===================================================================
--- src/RCF/ConnectionOrientedClientTransport.cpp	(revision 83459)
+++ src/RCF/ConnectionOrientedClientTransport.cpp	(working copy)
@@ -353,7 +353,9 @@
 
             mLastResponseSize += ret;
 
-            ByteBuffer b(mByteBuffer.release(), 0, ret);
+            ByteBuffer const &releasedBuffer = mByteBuffer.release();
+            ByteBuffer b(releasedBuffer, 0, ret);    
+                
             mTransportFilters.empty() ?
                 onReadCompleted(b) :
                 mTransportFilters.back()->onReadCompleted(b);

Where ByteBuffer's CCTOR has of course defaults on all the additional arguments
Comment 2 Douglas Gregor 2012-03-09 18:16:09 PST
Both GCC and Clang are correct here, even though they differ. Essentially, Clang is applying copy elision more aggressively than GCC, which is causing this code to do something different from what the author expected. Copy elision can be surprising that way, because it's explicitly stated that it can be applied even though it can change the result of programs.

Specifically, what's happening in the first case is that

  A other(original.makeA(), PASSED_IN);

is calling a copy constructor (since C++11 [class.copy]p2 considers that constructor a copy constructor). Then, C++11 [class.copy]p31 says that the compiler can omit the call to the copy constructor (3rd bullet):

  - when a temporary class object that has not been bound to a reference (12.2) would be copied/moved to a class object with the same cv-unqualified type, the copy/move operation can be omitted by constructing the temporary object directly into the target of the omitted copy/move
  
which means that the copy from the temporary "original.makeA()" can be elided, such that "other" directly gets the value of "original.makeA()". 

What does makeA() return? Well, we default-construct 'a' (so val has the value DEFAULT_CTOR) and then perform copy elision (the "named return value optimization", or NRVO) to end up directly returning that 'a' (without calling the copy constructor), so the resulting a have val == DEFAULT_CTOR.

The second case is similar. However, makeStaticA() behaves differently because while

  static A a;

gets the same val == DEFAULT_CTOR, the return statement cannot perform NRVO when returning a static local variable. Hence, the copy constructor gets called and we get back an object with val == DEFAULT_ARG.

The third and fourth cases don't involve copy elision, since we're coming from an lvalue and not a temporary.

Summing up, Clang is correctly performing copy elision, and the open source library in question will need to be fixed. I strongly suggest eliminating copy constructors that look like

  A(A const&, <parameters with default arguments>)

and instead splitting them into

  A(const A&); // copy constructor
  A(const A&, <parameters>); // copy + tweak constructor

so that differences in copy elision behavior between compilers don't continue to cause problems.
Comment 3 Sean Hunt 2012-03-09 19:10:15 PST
Reopening this bug as the fact that a copy constructor is being called is irrelevant if the construction is not, in fact, copy construction. p31 is applicable only during copy/move construction. See also CWG535 [1] which clarifies that elision can be applied to constructors other than copy and move constructors if they are selected to perform a copy or move operation, and was applied to the working draft at Kona.

[1]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3330.html#535
Comment 4 Douglas Gregor 2012-03-10 00:54:42 PST
(In reply to comment #3)
> Reopening this bug as the fact that a copy constructor is being called is
> irrelevant if the construction is not, in fact, copy construction. p31 is
> applicable only during copy/move construction. See also CWG535 [1] which
> clarifies that elision can be applied to constructors other than copy and move
> constructors if they are selected to perform a copy or move operation, and was
> applied to the working draft at Kona.
> 
> [1]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3330.html#535

Sean makes a good point. Fixed in Clang r152485.