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

diff --git a/lib/CodeGen/CGCall.cpp b/lib/CodeGen/CGCall.cpp
index 525697c3527536003f9a536ac0988c8cd3e1b939..233f6c17d485fea5e672cb7706d55f331a350362 100644 (file)
--- a/lib/CodeGen/CGCall.cpp
+++ b/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.
-  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);
@@ -3203,30 +3210,24 @@ void CodeGenFunction::EmitCallArgs(
       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);
   }
 }
 

diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp
index 63aa34678504fe4323632f9a3c64fcf5190b1bc8..7e12f5e73586650b04e81f47962d2a84fa56b682 100644 (file)
--- a/lib/CodeGen/CGExpr.cpp
+++ b/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);

diff --git a/lib/CodeGen/CGExprCXX.cpp b/lib/CodeGen/CGExprCXX.cpp
index 34ab6adad2ac8c4890cc18473af749058a93a4cb..4f54a0962f8a5a3ea3760154bbe83c7da0e174f4 100644 (file)
--- a/lib/CodeGen/CGExprCXX.cpp
+++ b/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);
     }
   }
 

diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h
index 378a2562f8a35a13a5b693bd649fcb1f9cb680de..db5798548866ddcf79b3eb8443ec9555658fa53f 100644 (file)
--- a/lib/CodeGen/CodeGenFunction.h
+++ b/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,
-                 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

diff --git a/test/CodeGenCXX/cxx1z-eval-order.cpp b/test/CodeGenCXX/cxx1z-eval-order.cpp
index 4b7c7f1061421610a30b16ae7f8032dd9ff1cb64..1106719a4748f8cabb4e7162fca41e678ea643a1 100644 (file)
--- a/test/CodeGenCXX/cxx1z-eval-order.cpp
+++ b/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();
 }

diff --git a/www/cxx_status.html b/www/cxx_status.html
index 1fe28d5dcbc7af6e35a2b0c55aa943481d11310e..170fc6090ef47efdc06a8e3a5746c7a407a9691e 100644 (file)
--- a/www/cxx_status.html
+++ 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,
-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>