https://github.com/xroche updated https://github.com/llvm/llvm-project/pull/215479
>From e713d2451ea8e6f569964fd9e829aee3d9aa28d5 Mon Sep 17 00:00:00 2001 From: Xavier Roche <[email protected]> Date: Mon, 10 Aug 2026 18:03:56 +0200 Subject: [PATCH] [Clang] Sequence the object argument of an overloaded operator[] or operator() first An operator declared with an explicit object parameter takes the object as an ordinary argument. Its operand order therefore comes from the prescribed-order switch in EmitCall, not the this-first member path. That switch had no OO_Subscript or OO_Call case. On Microsoft ABI targets the index or argument expressions were emitted before the object expression, against [expr.sub]/1 and [expr.call]/8, and for a C++23 multi-index subscript the object went last. The remaining operands are unsequenced among themselves, so ForceLeftToRight would constrain more than the standard asks and drag MS ABI parameter destruction order with it. Add a fourth EvaluationOrder for "first argument before the rest", used for both operators at any arity. The single-index subscript becomes a case of the same rule. Only the object argument is hoisted, and the rest keep the ABI's order. A static operator already emits and discards its object argument up front, so it is left alone. The switch now lists every operator with no default, so -Wswitch forces a decision on any operator added later. Assisted-by: Claude (Anthropic) Co-Authored-By: Claude Opus 5 <[email protected]> --- clang/docs/ReleaseNotes.md | 6 ++ clang/lib/CodeGen/CGCall.cpp | 26 ++++- clang/lib/CodeGen/CGExpr.cpp | 97 +++++++++++++++---- clang/lib/CodeGen/CodeGenFunction.h | 5 +- clang/test/CodeGenCXX/cxx1z-eval-order.cpp | 78 +++++++++++++++ .../CodeGenCXX/microsoft-abi-arg-order.cpp | 79 +++++++++++++++ 6 files changed, 265 insertions(+), 26 deletions(-) diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index a00b725143d49..0472f20377d89 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -386,6 +386,12 @@ features cannot lower the translation-unit ABI level; #### Bug Fixes to C++ Support +- Fixed the evaluation order of an overloaded `operator[]` or `operator()` + declared with an explicit object parameter on Microsoft ABI targets. The index + or argument expressions were evaluated before the object expression. Parameter + destruction order for such a call is no longer reverse construction order, as + with the other operators that have a prescribed operand order. + - Fixed an issue where `__typeof__` incorrectly rejected cv-qualified function types. - Fixed a bug where top-level CV qualifiers (such as ``const``) were dropped from pointers modified by Microsoft pointer attributes (like ``__ptr32`` and ``__ptr64``) and WebAssembly's ``__funcref``. diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index a29f7aab0e58b..9c6b4898bf580 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -5106,8 +5106,13 @@ void CodeGenFunction::EmitCallArgs( ? Order == EvaluationOrder::ForceLeftToRight : Order != EvaluationOrder::ForceRightToLeft; + // Only the first argument is hoisted out of the reversed run; the rest keep + // the ABI's order, which left-to-right emission already gives them. + bool HoistFirstArg = !LeftToRight && ArgTypes.size() > 1 && + Order == EvaluationOrder::ForceFirstBeforeRest; + auto MaybeEmitImplicitObjectSize = [&](unsigned I, const Expr *Arg, - RValue EmittedArg) { + RValue EmittedArg, bool Reversed) { if (!AC.hasFunctionDecl() || I >= AC.getNumParams()) return; auto *PS = AC.getParamDecl(I)->getAttr<PassObjectSizeAttr>(); @@ -5123,7 +5128,7 @@ void CodeGenFunction::EmitCallArgs( Args.add(RValue::get(V), SizeTy); // If we're emitting args in reverse, be sure to do so with // pass_object_size, as well. - if (!LeftToRight) + if (Reversed) std::swap(Args.back(), *(&Args.back() - 1)); }; @@ -5136,8 +5141,17 @@ void CodeGenFunction::EmitCallArgs( // Evaluate each argument in the appropriate order. size_t CallArgsStart = Args.size(); + // Start of the run of arguments emitted in reverse, past a hoisted first one. + size_t ReversedStart = CallArgsStart; for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) { - unsigned Idx = LeftToRight ? I : E - I - 1; + unsigned Idx; + if (LeftToRight) + Idx = I; + else if (HoistFirstArg) + Idx = I == 0 ? 0 : E - I; // 0, then E-1 down to 1. + else + Idx = E - I - 1; + bool Reversed = !LeftToRight && !(HoistFirstArg && I == 0); CallExpr::const_arg_iterator Arg = ArgRange.begin() + Idx; unsigned InitialArgSize = Args.size(); // If *Arg is an ObjCIndirectCopyRestoreExpr, check that either the types of @@ -5163,14 +5177,16 @@ void CodeGenFunction::EmitCallArgs( // @llvm.objectsize should never have side-effects and shouldn't need // destruction/cleanups, so we can safely "emit" it after its arg, // regardless of right-to-leftness - MaybeEmitImplicitObjectSize(Idx, *Arg, RVArg); + MaybeEmitImplicitObjectSize(Idx, *Arg, RVArg, Reversed); } + if (HoistFirstArg && I == 0) + ReversedStart = Args.size(); } 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()); + std::reverse(Args.begin() + ReversedStart, Args.end()); // Reverse the writebacks to match the MSVC ABI. Args.reverseWritebacks(); diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index 9201e40bc13a1..c313e55b6d4a3 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -7098,30 +7098,84 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType, if (Chain) Args.add(RValue::get(Chain), CGM.getContext().VoidPtrTy); - // C++17 requires that we evaluate arguments to a call using assignment syntax - // 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. + // C++17 [over.match.oper]/2 keeps the operands of an overloaded operator in + // the order prescribed for the built-in operator. 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; bool StaticOperator = false; if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(E)) { - if (OCE->isAssignmentOp()) + // Every operator is listed so that -Wswitch forces a decision on any + // operator added later. + switch (OCE->getOperator()) { + // The assignment operators sequence the right operand before the left. This + // list is also spelled out in CXXOperatorCallExpr::isAssignmentOp. + case OO_Equal: + case OO_PlusEqual: + case OO_MinusEqual: + case OO_StarEqual: + case OO_SlashEqual: + case OO_PercentEqual: + case OO_CaretEqual: + case OO_AmpEqual: + case OO_PipeEqual: + case OO_LessLessEqual: + case OO_GreaterGreaterEqual: 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; - } + break; + + // The shift, logical, comma and pointer-to-member operators sequence the + // left operand before the right. + case OO_LessLess: + case OO_GreaterGreater: + case OO_AmpAmp: + case OO_PipePipe: + case OO_Comma: + case OO_ArrowStar: + Order = EvaluationOrder::ForceLeftToRight; + break; + + // [expr.sub]/1 and [expr.call]/8 sequence the object argument before the + // index or argument expressions, which are unsequenced among themselves. + case OO_Subscript: + case OO_Call: + Order = EvaluationOrder::ForceFirstBeforeRest; + break; + + // No constraint on the operand order. + case OO_New: + case OO_Delete: + case OO_Array_New: + case OO_Array_Delete: + case OO_Plus: + case OO_Minus: + case OO_Star: + case OO_Slash: + case OO_Percent: + case OO_Caret: + case OO_Amp: + case OO_Pipe: + case OO_Tilde: + case OO_Exclaim: + case OO_Less: + case OO_Greater: + case OO_EqualEqual: + case OO_ExclaimEqual: + case OO_LessEqual: + case OO_GreaterEqual: + case OO_Spaceship: + case OO_PlusPlus: + case OO_MinusMinus: + case OO_Arrow: + case OO_Conditional: + case OO_Coawait: + break; + + case OO_None: + case NUM_OVERLOADED_OPERATORS: + llvm_unreachable("Not an overloaded operator"); } if (const auto *MD = @@ -7133,7 +7187,10 @@ RValue CodeGenFunction::EmitCall(QualType CalleeType, auto Arguments = E->arguments(); if (StaticOperator) { // If we're calling a static operator, we need to emit the object argument - // and ignore it. + // and ignore it. Emitting it here already sequences the object first, so no + // further constraint is needed. + if (Order == EvaluationOrder::ForceFirstBeforeRest) + Order = EvaluationOrder::Default; EmitIgnoredExpr(E->getArg(0)); Arguments = drop_begin(Arguments, 1); } diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 783f97dc354eb..4663fba4c3d42 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -5565,7 +5565,10 @@ class CodeGenFunction : public CodeGenTypeCache { ///! Language semantics require left-to-right evaluation. ForceLeftToRight, ///! Language semantics require right-to-left evaluation. - ForceRightToLeft + ForceRightToLeft, + ///! Language semantics require only the first argument to be evaluated + ///! first; the others are unsequenced. + ForceFirstBeforeRest }; // Wrapper for function prototype sources. Wraps either a FunctionProtoType or diff --git a/clang/test/CodeGenCXX/cxx1z-eval-order.cpp b/clang/test/CodeGenCXX/cxx1z-eval-order.cpp index 04c1b50f497e8..dca026d236d18 100644 --- a/clang/test/CodeGenCXX/cxx1z-eval-order.cpp +++ b/clang/test/CodeGenCXX/cxx1z-eval-order.cpp @@ -1,6 +1,9 @@ // RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple %itanium_abi_triple | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ITANIUM // RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple i686-windows | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-WINDOWS // RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-windows | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-WINDOWS +// RUN: %clang_cc1 -std=c++23 %s -emit-llvm -o - -triple %itanium_abi_triple | FileCheck %s --check-prefix=CXX23 --check-prefix=CXX23-ITANIUM +// RUN: %clang_cc1 -std=c++23 %s -emit-llvm -o - -triple i686-windows | FileCheck %s --check-prefix=CXX23 --check-prefix=CXX23-WINDOWS +// RUN: %clang_cc1 -std=c++23 %s -emit-llvm -o - -triple x86_64-windows | FileCheck %s --check-prefix=CXX23 --check-prefix=CXX23-WINDOWS struct B; struct A { @@ -269,3 +272,78 @@ void andor_lhs_before_rhs() { // CHECK: call {{.*}}@{{.*}}make_b{{.*}}( make_c() || make_b(); } + +// The C++23 cases live here rather than in a deducing-this test because the +// rule under test is operand order, not the explicit object parameter itself. +#if __cplusplus >= 202302L +// An operator with an explicit object parameter takes the object as an ordinary +// argument, so the object-before-rest order is not implied by the this-first +// emission path. +struct D { + void operator[](this D self, B b); + void operator[](this D self, B b, C c); + void operator[](this D self, B b, C c, A a); + void operator()(this D self, B b, C c); +}; +struct E { + static void operator()(B b, C c); +}; +D make_d(); +E make_e(); + +// One case per function, so a label bounds each order assertion. +// CXX23-LABEL: define {{.*}}@{{.*}}subscript_object_before_index{{.*}}( +void subscript_object_before_index() { + // CXX23: call {{.*}}@{{.*}}make_d{{.*}}( + // CXX23: call {{.*}}@{{.*}}make_b{{.*}}( + make_d()[make_b()]; +// CXX23: } +} + +// CXX23-LABEL: define {{.*}}@{{.*}}subscript_object_before_indices{{.*}}( +void subscript_object_before_indices() { + // The indices are unsequenced against each other, so they keep the ABI order. + // CXX23: call {{.*}}@{{.*}}make_d{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}( + make_d()[make_b(), make_c()]; +// CXX23: } +} + +// CXX23-LABEL: define {{.*}}@{{.*}}subscript_object_before_three_indices{{.*}}( +void subscript_object_before_three_indices() { + // CXX23: call {{.*}}@{{.*}}make_d{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_a{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_a{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}( + make_d()[make_b(), make_c(), make_a()]; +// CXX23: } +} + +// CXX23-LABEL: define {{.*}}@{{.*}}call_object_before_args{{.*}}( +void call_object_before_args() { + // CXX23: call {{.*}}@{{.*}}make_d{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}( + make_d()(make_b(), make_c()); +// CXX23: } +} + +// CXX23-LABEL: define {{.*}}@{{.*}}static_operator_object_first{{.*}}( +void static_operator_object_first() { + // CXX23: call {{.*}}@{{.*}}make_e{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_b{{.*}}( + // CXX23-ITANIUM: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_c{{.*}}( + // CXX23-WINDOWS: call {{.*}}@{{.*}}make_b{{.*}}( + make_e()(make_b(), make_c()); +// CXX23: } +} +#endif diff --git a/clang/test/CodeGenCXX/microsoft-abi-arg-order.cpp b/clang/test/CodeGenCXX/microsoft-abi-arg-order.cpp index 3f597705e0704..9b4a65e9f00ad 100644 --- a/clang/test/CodeGenCXX/microsoft-abi-arg-order.cpp +++ b/clang/test/CodeGenCXX/microsoft-abi-arg-order.cpp @@ -1,5 +1,7 @@ // RUN: %clang_cc1 -mconstructor-aliases -std=c++11 -fexceptions -emit-llvm %s -o - -triple=i386-pc-win32 | FileCheck %s -check-prefix=X86 // RUN: %clang_cc1 -mconstructor-aliases -std=c++11 -fexceptions -emit-llvm %s -o - -triple=x86_64-pc-win32 | FileCheck %s -check-prefix=X64 +// RUN: %clang_cc1 -mconstructor-aliases -std=c++23 -fexceptions -emit-llvm %s -o - -triple=i386-pc-win32 | FileCheck %s -check-prefix=X86-CXX23 +// RUN: %clang_cc1 -mconstructor-aliases -std=c++23 -fexceptions -emit-llvm %s -o - -triple=x86_64-pc-win32 | FileCheck %s -check-prefix=X64-CXX23 struct A { A(int a); @@ -74,3 +76,80 @@ void call_foo() { // // ehcleanup: // X64: call void @"??1A@@QEAA@XZ"(ptr {{[^,]*}} %[[arg3]]) + +#if __cplusplus >= 202302L +struct B { + B(int b); + B(const B &o); + ~B(); + int b; + void operator[](this B self, B i, B j); +}; + +void B::operator[](this B self, B i, B j) { +} + +// Order of destruction should be left to right. +// +// X86-CXX23-LABEL: define dso_local void @"??AB@@SAX_VU0@00@Z" +// X86-CXX23: (ptr inalloca([[argmem_b:<{ %struct.B, %struct.B, %struct.B }>]]) %0) +// X86-CXX23: %[[self:[^ ]*]] = getelementptr inbounds nuw [[argmem_b]], ptr %0, i32 0, i32 0 +// X86-CXX23: %[[i:[^ ]*]] = getelementptr inbounds nuw [[argmem_b]], ptr %0, i32 0, i32 1 +// X86-CXX23: %[[j:[^ ]*]] = getelementptr inbounds nuw [[argmem_b]], ptr %0, i32 0, i32 2 +// X86-CXX23: call x86_thiscallcc void @"??1B@@QAE@XZ"(ptr {{[^,]*}} %[[self]]) +// X86-CXX23: call x86_thiscallcc void @"??1B@@QAE@XZ"(ptr {{[^,]*}} %[[i]]) +// X86-CXX23: call x86_thiscallcc void @"??1B@@QAE@XZ"(ptr {{[^,]*}} %[[j]]) +// X86-CXX23: ret void + +// X64-CXX23-LABEL: define dso_local void @"??AB@@SAX_VU0@00@Z" +// X64-CXX23: (ptr {{[^,]*}} %[[self:[^,]*]], ptr {{[^,]*}} %[[i:[^,]*]], ptr {{[^,]*}} %[[j:[^)]*]]) +// X64-CXX23: call void @"??1B@@QEAA@XZ"(ptr {{[^,]*}} %[[self]]) +// X64-CXX23: call void @"??1B@@QEAA@XZ"(ptr {{[^,]*}} %[[i]]) +// X64-CXX23: call void @"??1B@@QEAA@XZ"(ptr {{[^,]*}} %[[j]]) +// X64-CXX23: ret void + + +void call_subscript() { + B(1)[B(2), B(3)]; +} + +// The object argument is evaluated first, the indices keep the right-to-left +// order, and we should clean up the right things as we unwind. +// +// X86-CXX23-LABEL: define dso_local void @"?call_subscript@@YAXXZ"() +// X86-CXX23: %[[argmem:[^ ]*]] = alloca inalloca [[argmem_b]] +// X86-CXX23: %[[obj:[^ ]*]] = getelementptr inbounds nuw [[argmem_b]], ptr %[[argmem]], i32 0, i32 0 +// X86-CXX23: call x86_thiscallcc noundef ptr @"??0B@@QAE@H@Z"(ptr {{[^,]*}} %[[obj]], i32 noundef 1) +// X86-CXX23: %[[idx2:[^ ]*]] = getelementptr inbounds nuw [[argmem_b]], ptr %[[argmem]], i32 0, i32 2 +// X86-CXX23: invoke x86_thiscallcc noundef ptr @"??0B@@QAE@H@Z"(ptr {{[^,]*}} %[[idx2]], i32 noundef 3) +// X86-CXX23: %[[idx1:[^ ]*]] = getelementptr inbounds nuw [[argmem_b]], ptr %[[argmem]], i32 0, i32 1 +// X86-CXX23: invoke x86_thiscallcc noundef ptr @"??0B@@QAE@H@Z"(ptr {{[^,]*}} %[[idx1]], i32 noundef 2) +// X86-CXX23: call void @"??AB@@SAX_VU0@00@Z"(ptr inalloca([[argmem_b]]) %[[argmem]]) +// X86-CXX23: ret void +// +// ehcleanup: +// X86-CXX23: cleanuppad within none [] +// X86-CXX23: call x86_thiscallcc void @"??1B@@QAE@XZ"(ptr {{[^,]*}} %[[idx2]]) +// X86-CXX23: cleanupret +// +// ehcleanup4: +// X86-CXX23: cleanuppad within none [] +// X86-CXX23: call x86_thiscallcc void @"??1B@@QAE@XZ"(ptr {{[^,]*}} %[[obj]]) + +// X64-CXX23-LABEL: define dso_local void @"?call_subscript@@YAXXZ"() +// X64-CXX23: call noundef ptr @"??0B@@QEAA@H@Z"(ptr {{[^,]*}} %[[obj:[^,]*]], i32 noundef 1) +// X64-CXX23: invoke noundef ptr @"??0B@@QEAA@H@Z"(ptr {{[^,]*}} %[[idx2:[^,]*]], i32 noundef 3) +// X64-CXX23: invoke noundef ptr @"??0B@@QEAA@H@Z"(ptr {{[^,]*}} %[[idx1:[^,]*]], i32 noundef 2) +// X64-CXX23: call void @"??AB@@SAX_VU0@00@Z" +// X64-CXX23: (ptr {{[^,]*}} %[[obj]], ptr {{[^,]*}} %[[idx1]], ptr {{[^,]*}} %[[idx2]]) +// X64-CXX23: ret void +// +// ehcleanup: +// X64-CXX23: cleanuppad within none [] +// X64-CXX23: call void @"??1B@@QEAA@XZ"(ptr {{[^,]*}} %[[idx2]]) +// X64-CXX23: cleanupret +// +// ehcleanup6: +// X64-CXX23: cleanuppad within none [] +// X64-CXX23: call void @"??1B@@QEAA@XZ"(ptr {{[^,]*}} %[[obj]]) +#endif _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
