Switch to a different workaround for unimplementability of P0145R3 in MS ABIs.

Instead of ignoring the evaluation order rule, ignore the "destroy parameters
in reverse construction order" rule for the small number of problematic cases.
This only causes incorrect behavior in the rare case where both parameters to
an overloaded operator <<, >>, ->*, &&, ||, or comma are of class type with
non-trivial destructor, and the program is depending on those parameters being
destroyed in reverse construction order.

We could do a little better here by reversing the order of parameter
destruction for those functions (and reversing the argument evaluation order
for all direct calls, not just those with operator syntax), but that is not a
complete solution to the problem, as the same situation can be reached by an
indirect function call.

Approach reviewed off-line by rnk.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@282777 91177308-0d34-0410-b5e6-96231b3b80d8

lib/CodeGen/CGCall.cpp

@@ -3173,7 +3173,7 @@ void CodeGenFunction::EmitCallArgs(
     CallArgList &Args, ArrayRef<QualType> ArgTypes,
     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
     const FunctionDecl *CalleeDecl, unsigned ParamsToSkip,
-    bool ForceRightToLeftEvaluation) {
+    EvaluationOrder Order) {
   assert((int)ArgTypes.size() == (ArgRange.end() - ArgRange.begin()));
 
   auto MaybeEmitImplicitObjectSize = [&](unsigned I, const Expr *Arg) {
@@ -3191,11 +3191,18 @@ 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.
+  // because arguments are destroyed left to right in the callee. As a special
-  if (CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee() ||
+  // case, there are certain language constructs that require left-to-right
-      ForceRightToLeftEvaluation) {
+  // evaluation, and in those cases we consider the evaluation order requirement
-    // Insert a stack save if we're going to need any inalloca args.
+  // to trump the "destruction order is reverse construction order" guarantee.
-    bool HasInAllocaArgs = false;
+  bool LeftToRight =
+      CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()
+          ? Order == EvaluationOrder::ForceLeftToRight
+          : Order != EvaluationOrder::ForceRightToLeft;
+
+  // Insert a stack save if we're going to need any inalloca args.
+  bool HasInAllocaArgs = false;
+  if (CGM.getTarget().getCXXABI().isMicrosoft()) {
     for (ArrayRef<QualType>::iterator I = ArgTypes.begin(), E = ArgTypes.end();
          I != E && !HasInAllocaArgs; ++I)
       HasInAllocaArgs = isInAllocaArgument(CGM.getCXXABI(), *I);
@@ -3203,30 +3210,24 @@ void CodeGenFunction::EmitCallArgs(
       assert(getTarget().getTriple().getArch() == llvm::Triple::x86);
       Args.allocateArgumentMemory(*this);
     }
+  }
 
-    // Evaluate each argument.
+  // Evaluate each argument in the appropriate order.
-    size_t CallArgsStart = Args.size();
+  size_t CallArgsStart = Args.size();
-    for (int I = ArgTypes.size() - 1; I >= 0; --I) {
+  for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) {
-      CallExpr::const_arg_iterator Arg = ArgRange.begin() + I;
+    unsigned Idx = LeftToRight ? I : E - I - 1;
-      MaybeEmitImplicitObjectSize(I, *Arg);
+    CallExpr::const_arg_iterator Arg = ArgRange.begin() + Idx;
-      EmitCallArg(Args, *Arg, ArgTypes[I]);
+    if (!LeftToRight) MaybeEmitImplicitObjectSize(Idx, *Arg);
-      EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
+    EmitCallArg(Args, *Arg, ArgTypes[Idx]);
-                          CalleeDecl, ParamsToSkip + I);
+    EmitNonNullArgCheck(Args.back().RV, ArgTypes[Idx], (*Arg)->getExprLoc(),
-    }
+                        CalleeDecl, ParamsToSkip + Idx);
+    if (LeftToRight) MaybeEmitImplicitObjectSize(Idx, *Arg);
+  }
 
+  if (!LeftToRight) {
     // Un-reverse the arguments we just evaluated so they match up with the LLVM
     // IR function.
     std::reverse(Args.begin() + CallArgsStart, Args.end());
-    return;
-  }
-
-  for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) {
-    CallExpr::const_arg_iterator Arg = ArgRange.begin() + I;
-    assert(Arg != ArgRange.end());
-    EmitCallArg(Args, *Arg, ArgTypes[I]);
-    EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
-                        CalleeDecl, ParamsToSkip + I);
-    MaybeEmitImplicitObjectSize(I, *Arg);
   }
 }
 

lib/CodeGen/CGExpr.cpp

@@ -4123,15 +4123,33 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType, llvm::Value *Callee,
              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
+  // right-to-left, and that we evaluate arguments to certain other operators
-  // <<, >>, and ->* left-to-right, but that is not possible under the MS ABI,
+  // left-to-right. Note that we allow this to override the order dictated by
-  // so there is no corresponding "force left-to-right" case.
+  // the calling convention on the MS ABI, which means that parameter
-  bool ForceRightToLeft = false;
+  // destruction order is not necessarily reverse construction order.
-  if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(E))
+  // FIXME: Revisit this based on C++ committee response to unimplementability.
-    ForceRightToLeft = OCE->isAssignmentOp();
+  EvaluationOrder Order = EvaluationOrder::Default;
+  if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(E)) {
+    if (OCE->isAssignmentOp())
+      Order = EvaluationOrder::ForceRightToLeft;
+    else {
+      switch (OCE->getOperator()) {
+      case OO_LessLess:
+      case OO_GreaterGreater:
+      case OO_AmpAmp:
+      case OO_PipePipe:
+      case OO_Comma:
+      case OO_ArrowStar:
+        Order = EvaluationOrder::ForceLeftToRight;
+        break;
+      default:
+        break;
+      }
+    }
+  }
 
   EmitCallArgs(Args, dyn_cast<FunctionProtoType>(FnType), E->arguments(),
-               E->getDirectCallee(), /*ParamsToSkip*/ 0, ForceRightToLeft);
+               E->getDirectCallee(), /*ParamsToSkip*/ 0, Order);
 
   const CGFunctionInfo &FnInfo = CGM.getTypes().arrangeFreeFunctionCall(
       Args, FnType, /*isChainCall=*/Chain);

lib/CodeGen/CGExprCXX.cpp

@@ -184,7 +184,7 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr(
       RtlArgs = &RtlArgStorage;
       EmitCallArgs(*RtlArgs, MD->getType()->castAs<FunctionProtoType>(),
                    drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
-                   /*ParamsToSkip*/0, /*ForceRightToLeftEvaluation*/true);
+                   /*ParamsToSkip*/0, EvaluationOrder::ForceRightToLeft);
     }
   }
 

lib/CodeGen/CodeGenFunction.h

@@ -3318,13 +3318,22 @@ public:
   static bool isObjCMethodWithTypeParams(const T *) { return false; }
 #endif
 
+  enum class EvaluationOrder {
+    ///! No language constraints on evaluation order.
+    Default,
+    ///! Language semantics require left-to-right evaluation.
+    ForceLeftToRight,
+    ///! Language semantics require right-to-left evaluation.
+    ForceRightToLeft
+  };
+
   /// EmitCallArgs - Emit call arguments for a function.
   template <typename T>
   void EmitCallArgs(CallArgList &Args, const T *CallArgTypeInfo,
                     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
                     const FunctionDecl *CalleeDecl = nullptr,
                     unsigned ParamsToSkip = 0,
-                    bool ForceRightToLeftEvaluation = false) {
+                    EvaluationOrder Order = EvaluationOrder::Default) {
     SmallVector<QualType, 16> ArgTypes;
     CallExpr::const_arg_iterator Arg = ArgRange.begin();
 
@@ -3364,15 +3373,14 @@ public:
     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, Order);
-                 ForceRightToLeftEvaluation);
   }
 
   void EmitCallArgs(CallArgList &Args, ArrayRef<QualType> ArgTypes,
                     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
                     const FunctionDecl *CalleeDecl = nullptr,
                     unsigned ParamsToSkip = 0,
-                    bool ForceRightToLeftEvaluation = false);
+                    EvaluationOrder Order = EvaluationOrder::Default);
 
   /// EmitPointerWithAlignment - Given an expression with a pointer
   /// type, emit the value and compute our best estimate of the

test/CodeGenCXX/cxx1z-eval-order.cpp

@@ -151,16 +151,15 @@ int dotstar_lhs_before_rhs() {
   // CHECK: call {{.*}}@{{.*}}make_a{{.*}}(
   make_c()->*make_a();
 
-  // FIXME: The corresponding case for Windows ABIs is unimplementable if the
+  // FIXME: For MS ABI, the order of destruction of parameters here will not be
-  // operand has a non-trivially-destructible type, because the order of
+  // reverse construction order (parameters are destroyed left-to-right in the
-  // construction of function arguments is defined by the ABI there (it's the
+  // callee). That sadly seems unavoidable; the rules are not implementable as
-  // reverse of the order in which the parameters are destroyed in the callee).
+  // specified. If we changed parameter destruction order for these functions
-  // But we could follow the C++17 rule in the case where the operand type is
+  // to right-to-left, we could make the destruction order match for all cases
-  // trivially-destructible.
+  // other than indirect calls, but we can't completely avoid the problem.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
+  //
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
   make_c()->*make_b();
 
   // CHECK: call {{.*}}@{{.*}}make_a{{.*}}(
@@ -228,17 +227,13 @@ void shift_lhs_before_rhs() {
   // CHECK: call {{.*}}@{{.*}}make_a{{.*}}(
   make_c() >> make_a();
 
-  // FIXME: This is unimplementable for Windows ABIs, see above.
+  // FIXME: This is not correct for Windows ABIs, see above.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
   make_c() << make_b();
 
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
   make_c() >> make_b();
 // CHECK: }
 }
@@ -249,11 +244,9 @@ void comma_lhs_before_rhs() {
   // CHECK: call {{.*}}@{{.*}}make_a{{.*}}(
   make_c() , make_a();
 
-  // FIXME: This is unimplementable for Windows ABIs, see above.
+  // FIXME: This is not correct for Windows ABIs, see above.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
   make_c() , make_b();
 }
 
@@ -267,16 +260,12 @@ void andor_lhs_before_rhs() {
   // CHECK: call {{.*}}@{{.*}}make_a{{.*}}(
   make_c() || make_a();
 
-  // FIXME: This is unimplementable for Windows ABIs, see above.
+  // FIXME: This is not correct for Windows ABIs, see above.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
   make_c() && make_b();
 
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
   make_c() || make_b();
 }

www/cxx_status.html

@@ -740,14 +740,12 @@ In Clang 3.7, a warning is emitted for all cases that would change meaning.
 <span id="p0136">(9): This is the resolution to a Defect Report, so is applied
 to all language versions supporting inheriting constructors.
 </span><br>
-<span id="p0145">(10): Under the MS ABI, this feature is not fully implementable,
+<span id="p0145">(10): Under the MS ABI, function parameters are destroyed from
-because the calling convention requires that function parameters are destroyed
+left to right in the callee. As a result, function parameters in calls to
-from left to right in the callee. In order to guarantee that destruction order
-is reverse construction order, the operands of overloaded
 <tt>operator&lt;&lt;</tt>, <tt>operator&gt;&gt;</tt>, <tt>operator-&gt;*</tt>,
 <tt>operator&amp;&amp;</tt>, <tt>operator||</tt>, and <tt>operator,</tt>
-functions are evaluated right-to-left under the MS ABI when called using operator
+functions using expression syntax are no longer guaranteed to be destroyed in
-syntax, not left-to-right as P0145R3 requires.
+reverse construction order in that ABI.
 </span>
 </p>
 </details>