Avoid eliding the first operand in the gnu "x ?: y" extension

[rdwampler]

Summary

Resolves https://bugs.llvm.org/show_bug.cgi?id=35039.

[arphaman]

Thanks for the patch!

Side-comment: I wonder if ever need to copy-elide OpaqueValues. There could be similar issues that happen to OpaqueValue expression that we haven't uncovered yet.

lib/Sema/SemaExprCXX.cpp

5637

Have you tried checking if LHS and Cond are the same instead of using the OpaqueValue to check for the BinaryConditionalOperator? The comment at the top of the function states that for gnu ?: operators LHS == Cond should be true.

test/CodeGenCXX/gnu-conditional-extension.cpp

1

Is it possible to create a C++1z Sema test as well? E.g. make copy & move constructor private for a struct so it should fail to compile when we try to copy the LHS?

[rdwampler]

In D39377#909945, @arphaman wrote:

Thanks for the patch!

Side-comment: I wonder if ever need to copy-elide OpaqueValues. There could be similar issues that happen to OpaqueValue expression that we haven't uncovered yet.

Actually, I submitted radar://35088001 which looks like this issues as well. In that case it was a problem with assigning a C++ object that's a property in a ObjC class using dot syntax E.g.

@interface A
@property CXXClass myCXXClass;
@end
...
A *a
a.myCXXClass = makeCXXClass();
...

Doing a simple check for OpaqueValues in TryConstructorInitialization appears to fix both problems and would eliminate the need for the special handling of each case (assuming we never elide OpaqueValues).

[rsmith]

I'm working on an alternative approach to fix this bug:

Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp       (revision 316821)
+++ lib/Sema/SemaExpr.cpp       (working copy)
@@ -7242,6 +7242,16 @@
       commonExpr = commonRes.get();
     }
 
+    // If the common expression is a class or array prvalue, materialize it
+    // so that we can safely refer to it multiple times.
+    if (commonExpr->isRValue() && (commonExpr->getType()->isClassType() ||
+                                   commonExpr->getType()->isArrayType())) {
+      ExprResult MatExpr = TemporaryMaterializationConversion(commonExpr);
+      if (MatExpr.isInvalid())
+        return ExprError();
+      commonExpr = MatExpr.get();
+    }
+
     opaqueValue = new (Context) OpaqueValueExpr(commonExpr->getExprLoc(),
                                                 commonExpr->getType(),
                                                 commonExpr->getValueKind(),

[rjmccall]

In D39377#910019, @rdwampler wrote:

In D39377#909945, @arphaman wrote:

Thanks for the patch!

Side-comment: I wonder if ever need to copy-elide OpaqueValues. There could be similar issues that happen to OpaqueValue expression that we haven't uncovered yet.

Actually, I submitted radar://35088001 which looks like this issues as well. In that case it was a problem with assigning a C++ object that's a property in a ObjC class using dot syntax E.g.

@interface A
@property CXXClass myCXXClass;
@end
...
A *a
a.myCXXClass = makeCXXClass();
...

Doing a simple check for OpaqueValues in TryConstructorInitialization appears to fix both problems and would eliminate the need for the special handling of each case (assuming we never elide OpaqueValues).

We should absolutely be able to elide the copy into the parameter here.

John.

include/clang/Sema/Initialization.h

enum EntityKind {
     EK_Member,
     /// \brief The entity being initialized is an element of an array.
     EK_ArrayElement,
     /// \brief The entity being initialized is an object (or array of
     /// objects) allocated via new.
     EK_New,
     /// \brief The entity being initialized is a temporary object.
     EK_Temporary,
+    // \brief The entity being initialized is first operand in the GNU "x ?: y"
+    // extension.
+    EK_GNUConditionalOperatorLHSElement,
     /// \brief The entity being initialized is a base member subobject.
     EK_Base,
     /// \brief The initialization is being done by a delegating constructor.
     EK_Delegating,
     /// \brief The entity being initialized is an element of a vector.
     /// or vector.
     EK_VectorElement,
     /// \brief The entity being initialized is a field of block descriptor for
public:
   
   /// \brief Create the initialization entity for a temporary.
   static InitializedEntity InitializeTemporary(TypeSourceInfo *TypeInfo,
                                                QualType Type) {
     InitializedEntity Result(EK_Temporary, SourceLocation(), Type);
     Result.TypeInfo = TypeInfo;
     return Result;
   }
-  
+  static InitializedEntity
+  InitializeGNUConditionalOperatorLHSElement(QualType Type) {
+    return InitializedEntity(EK_GNUConditionalOperatorLHSElement,
+                             SourceLocation(), Type);
+  }
+
   /// \brief Create the initialization entity for a related result.
   static InitializedEntity InitializeRelatedResult(ObjCMethodDecl *MD,
                                                    QualType Type) {
     InitializedEntity Result(EK_RelatedResult, SourceLocation(), Type);
     Result.MethodDecl = MD;
     return Result;
   }
 

lib/Sema/SemaExprCXX.cpp

QualType Sema::CXXCheckConditionalOperands(ExprResult &Cond, ExprResult &LHS,
   //   -- The second and third operands have the same type; the result
   //      is of that type. If the operands have class type, the result
   //      is a prvalue temporary of the result type, which is
   //      copy-initialized from either the second operand or the third
   //      operand depending on the value of the first operand.
   if (Context.getCanonicalType(LTy) == Context.getCanonicalType(RTy)) {
     if (LTy->isRecordType()) {
       // The operands have class type. Make a temporary copy.
-      InitializedEntity Entity = InitializedEntity::InitializeTemporary(LTy);
+      // C++17: In case of the gnu "x ?: y" extension, ensure we avoid eliding
+      // the LHS temporary.
+      ExprResult LHSCopy =
+          isa<OpaqueValueExpr>(LHS.get())
+              ? PerformCopyInitialization(
+                    InitializedEntity::
+                        InitializeGNUConditionalOperatorLHSElement(LTy),
+                    SourceLocation(), LHS)
+              : PerformCopyInitialization(
+                    InitializedEntity::InitializeTemporary(LTy),
+                    SourceLocation(), LHS);
 
-      ExprResult LHSCopy = PerformCopyInitialization(Entity,
-                                                     SourceLocation(),
-                                                     LHS);
       if (LHSCopy.isInvalid())
         return QualType();
 
-      ExprResult RHSCopy = PerformCopyInitialization(Entity,
-                                                     SourceLocation(),
-                                                     RHS);
+      ExprResult RHSCopy = PerformCopyInitialization(
+          InitializedEntity::InitializeTemporary(LTy), SourceLocation(), RHS);
+      
       if (RHSCopy.isInvalid())
         return QualType();
 
       LHS = LHSCopy;
       RHS = RHSCopy;
     }
 
     // If we have function pointer types, unify them anyway to unify their

lib/Sema/SemaInit.cpp

static void warnBracedScalarInit(Sema &S, const InitializedEntity &Entity,
   case InitializedEntity::EK_Variable:
   case InitializedEntity::EK_LambdaCapture:
     // No warning, might be direct-list-initialization.
     // FIXME: Should we warn for copy-list-initialization in these cases?
     break;
 
   case InitializedEntity::EK_New:
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_CompoundLiteralInit:
     // No warning, braces are part of the syntax of the underlying construct.
     break;
 
   case InitializedEntity::EK_RelatedResult:
     // No warning, we already warned when initializing the result.
     break;
 
DeclarationName InitializedEntity::getName() const {
 
   case EK_Variable:
   case EK_Member:
   case EK_Binding:
     return Variable.VariableOrMember->getDeclName();
 
   case EK_LambdaCapture:
     return DeclarationName(Capture.VarID);
-      
   case EK_Result:
   case EK_Exception:
   case EK_New:
   case EK_Temporary:
+  case EK_GNUConditionalOperatorLHSElement:
   case EK_Base:
   case EK_Delegating:
   case EK_ArrayElement:
   case EK_VectorElement:
   case EK_ComplexElement:
   case EK_BlockElement:
   case EK_LambdaToBlockConversionBlockElement:
   case EK_CompoundLiteralInit:
ValueDecl *InitializedEntity::getDecl() const {
   case EK_Parameter:
   case EK_Parameter_CF_Audited:
     return reinterpret_cast<ParmVarDecl*>(Parameter & ~0x1);
 
   case EK_Result:
   case EK_Exception:
   case EK_New:
   case EK_Temporary:
+  case EK_GNUConditionalOperatorLHSElement:
   case EK_Base:
   case EK_Delegating:
   case EK_ArrayElement:
   case EK_VectorElement:
   case EK_ComplexElement:
   case EK_BlockElement:
   case EK_LambdaToBlockConversionBlockElement:
   case EK_LambdaCapture:
bool InitializedEntity::allowsNRVO() const {
 
   case EK_Variable:
   case EK_Parameter:
   case EK_Parameter_CF_Audited:
   case EK_Member:
   case EK_Binding:
   case EK_New:
   case EK_Temporary:
+  case EK_GNUConditionalOperatorLHSElement:
   case EK_CompoundLiteralInit:
   case EK_Base:
   case EK_Delegating:
   case EK_ArrayElement:
   case EK_VectorElement:
   case EK_ComplexElement:
   case EK_BlockElement:
   case EK_LambdaToBlockConversionBlockElement:
unsigned InitializedEntity::dumpImpl(raw_ostream &OS) const {
   case EK_Parameter_CF_Audited: OS << "CF audited function Parameter";
     break;
   case EK_Result: OS << "Result"; break;
   case EK_Exception: OS << "Exception"; break;
   case EK_Member: OS << "Member"; break;
   case EK_Binding: OS << "Binding"; break;
   case EK_New: OS << "New"; break;
   case EK_Temporary: OS << "Temporary"; break;
+  case EK_GNUConditionalOperatorLHSElement:
+    OS << "ConditionalOperatorLHSElement";
+    break;
   case EK_CompoundLiteralInit: OS << "CompoundLiteral";break;
   case EK_RelatedResult: OS << "RelatedResult"; break;
   case EK_Base: OS << "Base"; break;
   case EK_Delegating: OS << "Delegating"; break;
   case EK_ArrayElement: OS << "ArrayElement " << Index; break;
   case EK_VectorElement: OS << "VectorElement " << Index; break;
   case EK_ComplexElement: OS << "ComplexElement " << Index; break;
   case EK_BlockElement: OS << "Block"; break;
static void TryConstructorInitialization(Sema &S,
   //     - If the initializer expression is a prvalue and the cv-unqualified
   //       version of the source type is the same class as the class of the
   //       destination, the initializer expression is used to initialize the
   //       destination object.
   // Per DR (no number yet), this does not apply when initializing a base
   // class or delegating to another constructor from a mem-initializer.
   // ObjC++: Lambda captured by the block in the lambda to block conversion
   // should avoid copy elision.
+  // GNU "x ?: y" extension: avoid eliding the LHS temporary.
   if (S.getLangOpts().CPlusPlus1z &&
       Entity.getKind() != InitializedEntity::EK_Base &&
       Entity.getKind() != InitializedEntity::EK_Delegating &&
       Entity.getKind() !=
           InitializedEntity::EK_LambdaToBlockConversionBlockElement &&
+      Entity.getKind() != 
+          InitializedEntity::EK_GNUConditionalOperatorLHSElement &&
       UnwrappedArgs.size() == 1 && UnwrappedArgs[0]->isRValue() &&
       S.Context.hasSameUnqualifiedType(UnwrappedArgs[0]->getType(), DestType)) {
     // Convert qualifications if necessary.
     Sequence.AddQualificationConversionStep(DestType, VK_RValue);
     if (ILE)
       Sequence.RewrapReferenceInitList(DestType, ILE);
     return;
   }
if (Entity.getDecl() &&
       return Sema::AA_Sending;
 
     return Sema::AA_Passing;
 
   case InitializedEntity::EK_Parameter_CF_Audited:
     if (Entity.getDecl() &&
       isa<ObjCMethodDecl>(Entity.getDecl()->getDeclContext()))
       return Sema::AA_Sending;
-      
     return !Diagnose ? Sema::AA_Passing : Sema::AA_Passing_CFAudited;
-      
   case InitializedEntity::EK_Result:
     return Sema::AA_Returning;
 
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_RelatedResult:
     // FIXME: Can we tell apart casting vs. converting?
     return Sema::AA_Casting;
 
   case InitializedEntity::EK_Member:
   case InitializedEntity::EK_Binding:
   case InitializedEntity::EK_ArrayElement:
   case InitializedEntity::EK_VectorElement:
static bool shouldBindAsTemporary(const InitializedEntity &Entity) {
   case InitializedEntity::EK_LambdaToBlockConversionBlockElement:
   case InitializedEntity::EK_LambdaCapture:
   case InitializedEntity::EK_CompoundLiteralInit:
     return false;
 
   case InitializedEntity::EK_Parameter:
   case InitializedEntity::EK_Parameter_CF_Audited:
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_RelatedResult:
   case InitializedEntity::EK_Binding:
     return true;
   }
 
   llvm_unreachable("missed an InitializedEntity kind?");
 }
 
case InitializedEntity::EK_LambdaCapture:
       return false;
 
     case InitializedEntity::EK_Member:
     case InitializedEntity::EK_Binding:
     case InitializedEntity::EK_Variable:
     case InitializedEntity::EK_Parameter:
     case InitializedEntity::EK_Parameter_CF_Audited:
     case InitializedEntity::EK_Temporary:
+    case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
     case InitializedEntity::EK_ArrayElement:
     case InitializedEntity::EK_Exception:
     case InitializedEntity::EK_CompoundLiteralInit:
     case InitializedEntity::EK_RelatedResult:
       return true;
   }
 
   llvm_unreachable("missed an InitializedEntity kind?");
case InitializedEntity::EK_Exception:
     return Entity.getThrowLoc();
 
   case InitializedEntity::EK_Variable:
   case InitializedEntity::EK_Binding:
     return Entity.getDecl()->getLocation();
 
   case InitializedEntity::EK_LambdaCapture:
     return Entity.getCaptureLoc();
-      
   case InitializedEntity::EK_ArrayElement:
   case InitializedEntity::EK_Member:
   case InitializedEntity::EK_Parameter:
   case InitializedEntity::EK_Parameter_CF_Audited:
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_New:
   case InitializedEntity::EK_Base:
   case InitializedEntity::EK_Delegating:
   case InitializedEntity::EK_VectorElement:
   case InitializedEntity::EK_ComplexElement:
   case InitializedEntity::EK_BlockElement:
   case InitializedEntity::EK_LambdaToBlockConversionBlockElement:
   case InitializedEntity::EK_CompoundLiteralInit:

 
 /// Returns true if the parameters describe a constructor initialization of
 /// an explicit temporary object, e.g. "Point(x, y)".
 static bool isExplicitTemporary(const InitializedEntity &Entity,
                                 const InitializationKind &Kind,
                                 unsigned NumArgs) {
   switch (Entity.getKind()) {
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_CompoundLiteralInit:
   case InitializedEntity::EK_RelatedResult:
     break;
   default:
     return false;
   }
 
   switch (Kind.getKind()) {
InitializedEntityOutlivesFullExpression(const InitializedEntity &Entity) {
   case InitializedEntity::EK_ComplexElement:
     // Could not determine what the full initialization is. Assume it might not
     // outlive the full-expression.
     return false;
 
   case InitializedEntity::EK_Parameter:
   case InitializedEntity::EK_Parameter_CF_Audited:
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_LambdaCapture:
   case InitializedEntity::EK_CompoundLiteralInit:
   case InitializedEntity::EK_RelatedResult:
     // The entity being initialized might not outlive the full-expression.
     return false;
   }
 
   llvm_unreachable("unknown entity kind");
case InitializedEntity::EK_Result:
     //      destroyed at the end of the full-expression in the return statement.
   case InitializedEntity::EK_New:
     //   -- A temporary bound to a reference in a new-initializer persists
     //      until the completion of the full-expression containing the
     //      new-initializer.
     return nullptr;
 
   case InitializedEntity::EK_Temporary:
+  case InitializedEntity::EK_GNUConditionalOperatorLHSElement:
   case InitializedEntity::EK_CompoundLiteralInit:
   case InitializedEntity::EK_RelatedResult:
     // We don't yet know the storage duration of the surrounding temporary.
     // Assume it's got full-expression duration for now, it will patch up our
     // storage duration if that's not correct.
     return nullptr;
 
   case InitializedEntity::EK_ArrayElement:

test/CodeGenCXX/gnu-conditional-extension.cpp

+// RUN: %clang_cc1 -std=c++1z -triple x86_64-apple-darwin10 -emit-llvm %s -o - | FileCheck %s
+
+struct Copyable {
+  Copyable();
+  Copyable(const Copyable &x);
+  operator bool();
+};
+
+Copyable makeCopyable() { return Copyable(); }
+
+Copyable avoidElidingCopy() {
+  Copyable C = makeCopyable() ?: Copyable();
+  return C;
+}
+
+struct NonCopyable {
+  NonCopyable();
+  NonCopyable(const NonCopyable &x) = delete;
+  NonCopyable(NonCopyable &&x);
+  operator bool();
+};
+
+NonCopyable makeNonCopyable() { return NonCopyable(); }
+
+NonCopyable avoidElidingMove() {
+  NonCopyable N = makeNonCopyable() ?: NonCopyable();
+  return N;
+}
+
+// CHECK-LABEL: define void @_Z16avoidElidingCopyv(%struct.Copyable* noalias sret %agg.result)
+// CHECK: call void @_ZN8CopyableC1ERKS_(%struct.Copyable* %agg.result, %struct.Copyable* dereferenceable(1) %tmp)
+// CHECK-LABEL: define void @_Z16avoidElidingMovev(%struct.NonCopyable* noalias sret %agg.result)
+// CHECK: call void @_ZN11NonCopyableC1EOS_(%struct.NonCopyable* %agg.result, %struct.NonCopyable* dereferenceable(1) %tmp)