r282777 - Switch to a different workaround for unimplementability of P0145R3 in MS ABIs.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 29 14:30:12 PDT 2016
Author: rsmith
Date: Thu Sep 29 16:30:12 2016
New Revision: 282777
URL: http://llvm.org/viewvc/llvm-project?rev=282777&view=rev
Log:
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.
Modified:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprCXX.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/test/CodeGenCXX/cxx1z-eval-order.cpp
cfe/trunk/www/cxx_status.html
Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=282777&r1=282776&r2=282777&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Thu Sep 29 16:30:12 2016
@@ -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);
}
}
Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=282777&r1=282776&r2=282777&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Thu Sep 29 16:30:12 2016
@@ -4123,15 +4123,33 @@ RValue CodeGenFunction::EmitCall(QualTyp
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);
Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=282777&r1=282776&r2=282777&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Thu Sep 29 16:30:12 2016
@@ -184,7 +184,7 @@ RValue CodeGenFunction::EmitCXXMemberOrO
RtlArgs = &RtlArgStorage;
EmitCallArgs(*RtlArgs, MD->getType()->castAs<FunctionProtoType>(),
drop_begin(CE->arguments(), 1), CE->getDirectCallee(),
- /*ParamsToSkip*/0, /*ForceRightToLeftEvaluation*/true);
+ /*ParamsToSkip*/0, EvaluationOrder::ForceRightToLeft);
}
}
Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=282777&r1=282776&r2=282777&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Thu Sep 29 16:30:12 2016
@@ -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
Modified: cfe/trunk/test/CodeGenCXX/cxx1z-eval-order.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/cxx1z-eval-order.cpp?rev=282777&r1=282776&r2=282777&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenCXX/cxx1z-eval-order.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/cxx1z-eval-order.cpp Thu Sep 29 16:30:12 2016
@@ -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();
}
Modified: cfe/trunk/www/cxx_status.html
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/www/cxx_status.html?rev=282777&r1=282776&r2=282777&view=diff
==============================================================================
--- cfe/trunk/www/cxx_status.html (original)
+++ cfe/trunk/www/cxx_status.html Thu Sep 29 16:30:12 2016
@@ -740,14 +740,12 @@ In Clang 3.7, a warning is emitted for a
<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<<</tt>, <tt>operator>></tt>, <tt>operator->*</tt>,
<tt>operator&&</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>
More information about the cfe-commits
mailing list