Re-commit r282556, reverted in r282564, with a fix to CallArgList::addFrom to

function correctly when targeting MS ABIs (this appears to have never mattered
prior to this change).

Update test case to always cover both 32-bit and 64-bit Windows ABIs, since
they behave somewhat differently from each other here.

Update test case to also cover operators , && and ||, which it appears are also
affected by P0145R3 (they're not explicitly called out by the design document,
but this is the emergent behavior of the existing wording).


Original commit message:

P0145R3 (C++17 evaluation order tweaks): evaluate the right-hand side of
assignment and compound-assignment operators before the left-hand side. (Even
if it's an overloaded operator.)

This completes the implementation of P0145R3 + P0400R0 for all targets except
Windows, where the evaluation order guarantees for <<, >>, and ->* are
unimplementable as the ABI requires the function arguments are evaluated from
right to left (because parameter destructors are run from left to right in the
callee).

llvm-svn: 282619

Author: zygoloid

clang/include/clang/AST/ExprCXX.h

@@ -76,6 +76,16 @@ class CXXOperatorCallExpr : public CallExpr {
   /// expression refers to.
   OverloadedOperatorKind getOperator() const { return Operator; }
 
+  static bool isAssignmentOp(OverloadedOperatorKind Opc) {
+    return Opc == OO_Equal || Opc == OO_StarEqual ||
+           Opc == OO_SlashEqual || Opc == OO_PercentEqual ||
+           Opc == OO_PlusEqual || Opc == OO_MinusEqual ||
+           Opc == OO_LessLessEqual || Opc == OO_GreaterGreaterEqual ||
+           Opc == OO_AmpEqual || Opc == OO_CaretEqual ||
+           Opc == OO_PipeEqual;
+  }
+  bool isAssignmentOp() const { return isAssignmentOp(getOperator()); }
+
   /// \brief Returns the location of the operator symbol in the expression.
   ///
   /// When \c getOperator()==OO_Call, this is the location of the right

clang/lib/CodeGen/CGCall.cpp

@@ -3129,7 +3129,7 @@ static void emitWritebackArg(CodeGenFunction &CGF, CallArgList &args,
 }
 
 void CallArgList::allocateArgumentMemory(CodeGenFunction &CGF) {
-  assert(!StackBase && !StackCleanup.isValid());
+  assert(!StackBase);
 
   // Save the stack.
   llvm::Function *F = CGF.CGM.getIntrinsic(llvm::Intrinsic::stacksave);
@@ -3172,7 +3172,8 @@ void CodeGenFunction::EmitNonNullArgCheck(RValue RV, QualType ArgType,
 void CodeGenFunction::EmitCallArgs(
     CallArgList &Args, ArrayRef<QualType> ArgTypes,
     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
-    const FunctionDecl *CalleeDecl, unsigned ParamsToSkip) {
+    const FunctionDecl *CalleeDecl, unsigned ParamsToSkip,
+    bool ForceRightToLeftEvaluation) {
   assert((int)ArgTypes.size() == (ArgRange.end() - ArgRange.begin()));
 
   auto MaybeEmitImplicitObjectSize = [&](unsigned I, const Expr *Arg) {
@@ -3191,7 +3192,8 @@ void CodeGenFunction::EmitCallArgs(
 
   // We *have* to evaluate arguments from right to left in the MS C++ ABI,
   // because arguments are destroyed left to right in the callee.
-  if (CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) {
+  if (CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee() ||
+      ForceRightToLeftEvaluation) {
     // Insert a stack save if we're going to need any inalloca args.
     bool HasInAllocaArgs = false;
     for (ArrayRef<QualType>::iterator I = ArgTypes.begin(), E = ArgTypes.end();

clang/lib/CodeGen/CGCall.h

@@ -81,10 +81,19 @@ namespace CodeGen {
       push_back(CallArg(rvalue, type, needscopy));
     }
 
+    /// Add all the arguments from another CallArgList to this one. After doing
+    /// this, the old CallArgList retains its list of arguments, but must not
+    /// be used to emit a call.
     void addFrom(const CallArgList &other) {
       insert(end(), other.begin(), other.end());
       Writebacks.insert(Writebacks.end(),
                         other.Writebacks.begin(), other.Writebacks.end());
+      CleanupsToDeactivate.insert(CleanupsToDeactivate.end(),
+                                  other.CleanupsToDeactivate.begin(),
+                                  other.CleanupsToDeactivate.end());
+      assert(!(StackBase && other.StackBase) && "can't merge stackbases");
+      if (!StackBase)
+        StackBase = other.StackBase;
     }
 
     void addWriteback(LValue srcLV, Address temporary,
@@ -132,11 +141,6 @@ namespace CodeGen {
 
     /// The stacksave call.  It dominates all of the argument evaluation.
     llvm::CallInst *StackBase;
-
-    /// The iterator pointing to the stack restore cleanup.  We manually run and
-    /// deactivate this cleanup after the call in the unexceptional case because
-    /// it doesn't run in the normal order.
-    EHScopeStack::stable_iterator StackCleanup;
   };
 
   /// FunctionArgList - Type for representing both the decl and type

clang/lib/CodeGen/CGExpr.cpp

@@ -4121,8 +4121,17 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType, llvm::Value *Callee,
   if (Chain)
     Args.add(RValue::get(Builder.CreateBitCast(Chain, CGM.VoidPtrTy)),
              CGM.getContext().VoidPtrTy);
+
+  // C++17 requires that we evaluate arguments to a call using assignment syntax
+  // right-to-left. It also requires that we evaluate arguments to operators
+  // <<, >>, and ->* left-to-right, but that is not possible under the MS ABI,
+  // so there is no corresponding "force left-to-right" case.
+  bool ForceRightToLeft = false;
+  if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(E))
+    ForceRightToLeft = OCE->isAssignmentOp();
+
   EmitCallArgs(Args, dyn_cast<FunctionProtoType>(FnType), E->arguments(),
-               E->getDirectCallee(), /*ParamsToSkip*/ 0);
+               E->getDirectCallee(), /*ParamsToSkip*/ 0, ForceRightToLeft);
 
   const CGFunctionInfo &FnInfo = CGM.getTypes().arrangeFreeFunctionCall(
       Args, FnType, /*isChainCall=*/Chain);

clang/lib/CodeGen/CGExprCXX.cpp

@@ -28,7 +28,7 @@ static RequiredArgs
 commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD,
                                   llvm::Value *This, llvm::Value *ImplicitParam,
                                   QualType ImplicitParamTy, const CallExpr *CE,
-                                  CallArgList &Args) {
+                                  CallArgList &Args, CallArgList *RtlArgs) {
   assert(CE == nullptr || isa<CXXMemberCallExpr>(CE) ||
          isa<CXXOperatorCallExpr>(CE));
   assert(MD->isInstance() &&
@@ -61,7 +61,12 @@ commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD,
   RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size(), MD);
 
   // And the rest of the call args.
-  if (CE) {
+  if (RtlArgs) {
+    // Special case: if the caller emitted the arguments right-to-left already
+    // (prior to emitting the *this argument), we're done. This happens for
+    // assignment operators.
+    Args.addFrom(*RtlArgs);
+  } else if (CE) {
     // Special case: skip first argument of CXXOperatorCall (it is "this").
     unsigned ArgsToSkip = isa<CXXOperatorCallExpr>(CE) ? 1 : 0;
     CGF.EmitCallArgs(Args, FPT, drop_begin(CE->arguments(), ArgsToSkip),
@@ -77,11 +82,11 @@ commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD,
 RValue CodeGenFunction::EmitCXXMemberOrOperatorCall(
     const CXXMethodDecl *MD, llvm::Value *Callee, ReturnValueSlot ReturnValue,
     llvm::Value *This, llvm::Value *ImplicitParam, QualType ImplicitParamTy,
-    const CallExpr *CE) {
+    const CallExpr *CE, CallArgList *RtlArgs) {
   const FunctionProtoType *FPT = MD->getType()->castAs<FunctionProtoType>();
   CallArgList Args;
   RequiredArgs required = commonEmitCXXMemberOrOperatorCall(
-      *this, MD, This, ImplicitParam, ImplicitParamTy, CE, Args);
+      *this, MD, This, ImplicitParam, ImplicitParamTy, CE, Args, RtlArgs);
   return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required),
                   Callee, ReturnValue, Args, MD);
 }
@@ -92,7 +97,7 @@ RValue CodeGenFunction::EmitCXXDestructorCall(
     StructorType Type) {
   CallArgList Args;
   commonEmitCXXMemberOrOperatorCall(*this, DD, This, ImplicitParam,
-                                    ImplicitParamTy, CE, Args);
+                                    ImplicitParamTy, CE, Args, nullptr);
   return EmitCall(CGM.getTypes().arrangeCXXStructorDeclaration(DD, Type),
                   Callee, ReturnValueSlot(), Args, DD);
 }
@@ -170,6 +175,19 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
     }
   }
 
+  // C++17 demands that we evaluate the RHS of a (possibly-compound) assignment
+  // operator before the LHS.
+  CallArgList RtlArgStorage;
+  CallArgList *RtlArgs = nullptr;
+  if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(CE)) {
+    if (OCE->isAssignmentOp()) {
+      RtlArgs = &RtlArgStorage;
+      EmitCallArgs(*RtlArgs, MD->getType()->castAs<FunctionProtoType>(),
+                   drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
+                   /*ParamsToSkip*/0, /*ForceRightToLeftEvaluation*/true);
+    }
+  }
+
   Address This = Address::invalid();
   if (IsArrow)
     This = EmitPointerWithAlignment(Base);
@@ -187,10 +205,12 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
       if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) {
         // We don't like to generate the trivial copy/move assignment operator
         // when it isn't necessary; just produce the proper effect here.
-        // Special case: skip first argument of CXXOperatorCall (it is "this").
-        unsigned ArgsToSkip = isa<CXXOperatorCallExpr>(CE) ? 1 : 0;
-        Address RHS = EmitLValue(*(CE->arg_begin() + ArgsToSkip)).getAddress();
-        EmitAggregateAssign(This, RHS, CE->getType());
+        LValue RHS = isa<CXXOperatorCallExpr>(CE)
+                         ? MakeNaturalAlignAddrLValue(
+                               (*RtlArgs)[0].RV.getScalarVal(),
+                               (*(CE->arg_begin() + 1))->getType())
+                         : EmitLValue(*CE->arg_begin());
+        EmitAggregateAssign(This, RHS.getAddress(), CE->getType());
         return RValue::get(This.getPointer());
       }
 
@@ -249,7 +269,8 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
         Callee = CGM.GetAddrOfFunction(GlobalDecl(DDtor, Dtor_Complete), Ty);
       }
       EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This.getPointer(),
-                                  /*ImplicitParam=*/nullptr, QualType(), CE);
+                                  /*ImplicitParam=*/nullptr, QualType(), CE,
+                                  nullptr);
     }
     return RValue::get(nullptr);
   }
@@ -282,7 +303,8 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
   }
 
   return EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This.getPointer(),
-                                     /*ImplicitParam=*/nullptr, QualType(), CE);
+                                     /*ImplicitParam=*/nullptr, QualType(), CE,
+                                     RtlArgs);
 }
 
 RValue

clang/lib/CodeGen/CodeGenFunction.h

@@ -2867,7 +2867,8 @@ class CodeGenFunction : public CodeGenTypeCache {
   EmitCXXMemberOrOperatorCall(const CXXMethodDecl *MD, llvm::Value *Callee,
                               ReturnValueSlot ReturnValue, llvm::Value *This,
                               llvm::Value *ImplicitParam,
-                              QualType ImplicitParamTy, const CallExpr *E);
+                              QualType ImplicitParamTy, const CallExpr *E,
+                              CallArgList *RtlArgs);
   RValue EmitCXXDestructorCall(const CXXDestructorDecl *DD, llvm::Value *Callee,
                                llvm::Value *This, llvm::Value *ImplicitParam,
                                QualType ImplicitParamTy, const CallExpr *E,
@@ -3322,7 +3323,8 @@ class CodeGenFunction : public CodeGenTypeCache {
   void EmitCallArgs(CallArgList &Args, const T *CallArgTypeInfo,
                     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
                     const FunctionDecl *CalleeDecl = nullptr,
-                    unsigned ParamsToSkip = 0) {
+                    unsigned ParamsToSkip = 0,
+                    bool ForceRightToLeftEvaluation = false) {
     SmallVector<QualType, 16> ArgTypes;
     CallExpr::const_arg_iterator Arg = ArgRange.begin();
 
@@ -3362,13 +3364,15 @@ class CodeGenFunction : public CodeGenTypeCache {
     for (auto *A : llvm::make_range(Arg, ArgRange.end()))
       ArgTypes.push_back(getVarArgType(A));
 
-    EmitCallArgs(Args, ArgTypes, ArgRange, CalleeDecl, ParamsToSkip);
+    EmitCallArgs(Args, ArgTypes, ArgRange, CalleeDecl, ParamsToSkip,
+                 ForceRightToLeftEvaluation);
   }
 
   void EmitCallArgs(CallArgList &Args, ArrayRef<QualType> ArgTypes,
                     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
                     const FunctionDecl *CalleeDecl = nullptr,
-                    unsigned ParamsToSkip = 0);
+                    unsigned ParamsToSkip = 0,
+                    bool ForceRightToLeftEvaluation = false);
 
   /// EmitPointerWithAlignment - Given an expression with a pointer
   /// type, emit the value and compute our best estimate of the

clang/lib/CodeGen/ItaniumCXXABI.cpp

@@ -1456,7 +1456,8 @@ void ItaniumCXXABI::EmitDestructorCall(CodeGenFunction &CGF,
     Callee = CGM.getAddrOfCXXStructor(DD, getFromDtorType(Type));
 
   CGF.EmitCXXMemberOrOperatorCall(DD, Callee, ReturnValueSlot(),
-                                  This.getPointer(), VTT, VTTTy, nullptr);
+                                  This.getPointer(), VTT, VTTTy,
+                                  nullptr, nullptr);
 }
 
 void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
@@ -1636,7 +1637,7 @@ llvm::Value *ItaniumCXXABI::EmitVirtualDestructorCall(
 
   CGF.EmitCXXMemberOrOperatorCall(Dtor, Callee, ReturnValueSlot(),
                                   This.getPointer(), /*ImplicitParam=*/nullptr,
-                                  QualType(), CE);
+                                  QualType(), CE, nullptr);
   return nullptr;
 }