Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLANG copy ctor with default arguments disregard passed-in arguments when invoked with an Rvalue source object #12511

Closed
llvmbot opened this issue Feb 29, 2012 · 4 comments
Labels
bugzilla Issues migrated from bugzilla c++ miscompilation

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 29, 2012

Bugzilla Link 12139
Resolution FIXED
Resolved on Oct 05, 2012 20:37
Version trunk
OS MacOS X
Reporter LLVM Bugzilla Contributor
CC @DougGregor

Extended Description

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
#include
#include
#include

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Mar 1, 2012

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

@DougGregor
Copy link
Contributor

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&, )

and instead splitting them into

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

so that differences in copy elision behavior between compilers don't continue to cause problems.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Mar 10, 2012

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.

@DougGregor
Copy link
Contributor

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.

Sean makes a good point. Fixed in Clang r152485.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla c++ miscompilation
Projects
None yet
Development

No branches or pull requests

2 participants