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.

llvm-svn: 282777

clang/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,42 +3191,43 @@ 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() ||
-      ForceRightToLeftEvaluation) {
-    // Insert a stack save if we're going to need any inalloca args.
-    bool HasInAllocaArgs = false;
+  // because arguments are destroyed left to right in the callee. As a special
+  // case, there are certain language constructs that require left-to-right
+  // evaluation, and in those cases we consider the evaluation order requirement
+  // to trump the "destruction order is reverse construction order" guarantee.
+  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);
     if (HasInAllocaArgs) {
       assert(getTarget().getTriple().getArch() == llvm::Triple::x86);
       Args.allocateArgumentMemory(*this);
     }
+  }
 
-    // Evaluate each argument.
-    size_t CallArgsStart = Args.size();
-    for (int I = ArgTypes.size() - 1; I >= 0; --I) {
-      CallExpr::const_arg_iterator Arg = ArgRange.begin() + I;
-      MaybeEmitImplicitObjectSize(I, *Arg);
-      EmitCallArg(Args, *Arg, ArgTypes[I]);
-      EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], (*Arg)->getExprLoc(),
-                          CalleeDecl, ParamsToSkip + I);
-    }
+  // Evaluate each argument in the appropriate order.
+  size_t CallArgsStart = Args.size();
+  for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) {
+    unsigned Idx = LeftToRight ? I : E - I - 1;
+    CallExpr::const_arg_iterator Arg = ArgRange.begin() + Idx;
+    if (!LeftToRight) MaybeEmitImplicitObjectSize(Idx, *Arg);
+    EmitCallArg(Args, *Arg, ArgTypes[Idx]);
+    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);
   }
 }
 

clang/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
-  // <<, >>, 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();
+  // right-to-left, and that we evaluate arguments to certain other operators
+  // left-to-right. Note that we allow this to override the order dictated by
+  // the calling convention on the MS ABI, which means that parameter
+  // destruction order is not necessarily reverse construction order.
+  // FIXME: Revisit this based on C++ committee response to unimplementability.
+  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);

clang/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);
     }
   }
 

clang/lib/CodeGen/CodeGenFunction.h

@@ -3318,13 +3318,22 @@ class CodeGenFunction : public CodeGenTypeCache {
   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 @@ class CodeGenFunction : public CodeGenTypeCache {
     for (auto *A : llvm::make_range(Arg, ArgRange.end()))
       ArgTypes.push_back(getVarArgType(A));
 
-    EmitCallArgs(Args, ArgTypes, ArgRange, CalleeDecl, ParamsToSkip,
-                 ForceRightToLeftEvaluation);
+    EmitCallArgs(Args, ArgTypes, ArgRange, CalleeDecl, ParamsToSkip, Order);
   }
 
   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

clang/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
-  // operand has a non-trivially-destructible type, because the order of
-  // construction of function arguments is defined by the ABI there (it's the
-  // reverse of the order in which the parameters are destroyed in the callee).
-  // But we could follow the C++17 rule in the case where the operand type is
-  // trivially-destructible.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
+  // FIXME: For MS ABI, the order of destruction of parameters here will not be
+  // reverse construction order (parameters are destroyed left-to-right in the
+  // callee). That sadly seems unavoidable; the rules are not implementable as
+  // specified. If we changed parameter destruction order for these functions
+  // to right-to-left, we could make the destruction order match for all cases
+  // other than indirect calls, but we can't completely avoid the problem.
+  //
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
   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.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
+  // FIXME: This is not correct for Windows ABIs, see above.
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
   make_c() << make_b();
 
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
   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.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
+  // FIXME: This is not correct for Windows ABIs, see above.
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
   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.
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
+  // FIXME: This is not correct for Windows ABIs, see above.
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
   make_c() && make_b();
 
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}(
-  // CHECK-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}(
-  // CHECK-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_c{{.*}}(
+  // CHECK: call {{.*}}@{{.*}}make_b{{.*}}(
   make_c() || make_b();
 }

clang/www/cxx_status.html

@@ -740,14 +740,12 @@ <h2 id="cxx17">C++1z implementation status</h2>
 <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,
-because the calling convention requires that function parameters are destroyed
-from left to right in the callee. In order to guarantee that destruction order
-is reverse construction order, the operands of overloaded
+<span id="p0145">(10): Under the MS ABI, function parameters are destroyed from
+left to right in the callee. As a result, function parameters in calls to
 <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
-syntax, not left-to-right as P0145R3 requires.
+functions using expression syntax are no longer guaranteed to be destroyed in
+reverse construction order in that ABI.
 </span>
 </p>
 </details>