Author: Andy Kaylor Date: 2026-09-22T13:46:06-07:00 New Revision: 365851e9fe1b8723a0282e3849470cd7cbbe38d6
URL: https://github.com/llvm/llvm-project/commit/365851e9fe1b8723a0282e3849470cd7cbbe38d6 DIFF: https://github.com/llvm/llvm-project/commit/365851e9fe1b8723a0282e3849470cd7cbbe38d6.diff LOG: [CIR] Fix a problem with cleanup active flag initialization (#225507) While working on another problem related to conditional cleanups, I came across a problem with the way CIR was initializing the cleanup active flag. We were often clearing the flag below the body of a conditional cleanup region that the flag was meant to guard. As a result, we would sometimes skip a conditional cleanup becuase the body of the cleanup region set the active flag to true, but then we cleared the flag unconditionally before control flow reached the cleanup. This change fixes that problem and aligns the active flag initialization placement with classic codegen. Assisted-by: Cursor / various models Added: Modified: clang/lib/CIR/CodeGen/CIRGenCleanup.cpp clang/lib/CIR/CodeGen/CIRGenExpr.cpp clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp clang/lib/CIR/CodeGen/CIRGenExprComplex.cpp clang/lib/CIR/CodeGen/CIRGenFunction.h clang/test/CIR/CodeGen/cleanup-conditional-eh.cpp clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper-eh.cpp clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper.cpp clang/test/CIR/CodeGen/cleanup-conditional.cpp Removed: ################################################################################ diff --git a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp index 8103ef37f9225..161743dd82656 100644 --- a/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenCleanup.cpp @@ -85,7 +85,9 @@ Address CIRGenFunction::createCleanupActiveFlag() { { mlir::OpBuilder::InsertionGuard guard(builder); builder.restoreInsertionPoint(outermostConditional->getInsertPoint()); - builder.createFlagStore(loc, false, active.getPointer()); + cir::StoreOp store = + builder.createFlagStore(loc, false, active.getPointer()); + outermostConditional->advanceInsertPoint(store); } // Set to true at the current location (inside the conditional branch). diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp index 664425b20577f..a8feefe27b109 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp @@ -3063,12 +3063,13 @@ CIRGenFunction::ConditionalInfo CIRGenFunction::emitConditionalBlocks(const AbstractConditionalOperator *e, const FuncTy &branchGenFunc) { ConditionalInfo info; - ConditionalEvaluation eval(*this); mlir::Location loc = getLoc(e->getSourceRange()); CIRGenBuilderTy &builder = getBuilder(); mlir::Value condV = emitOpOnBoolExpr(loc, e->getCond()); + ConditionalEvaluation eval(*this); + auto emitBranch = [&](mlir::OpBuilder &b, mlir::Location loc, const Expr *expr, std::optional<LValue> &resultLV) { CIRGenFunction::LexicalScope lexScope{*this, loc, b.getInsertionBlock()}; diff --git a/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp b/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp index d95dfe6f2a6e2..d0105c96cfd39 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp @@ -1776,8 +1776,9 @@ mlir::Value CIRGenFunction::emitCXXNewExpr(const CXXNewExpr *e) { // conditionally (with an active flag) after the branch. The enclosing // FullExprCleanupScope detects this via ConditionalEvaluationFinder and // provides the cleanup region for the deferred destructors. - ConditionalEvaluation eval(*this); mlir::Value isNotNull = builder.createPtrIsNotNull(allocation.getPointer()); + + ConditionalEvaluation eval(*this); nullCheckOp = cir::IfOp::create(builder, getLoc(e->getSourceRange()), isNotNull, /*withElseRegion=*/false, diff --git a/clang/lib/CIR/CodeGen/CIRGenExprComplex.cpp b/clang/lib/CIR/CodeGen/CIRGenExprComplex.cpp index 1fab2352b10ce..135178b95a466 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprComplex.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprComplex.cpp @@ -1003,11 +1003,11 @@ mlir::Value ComplexExprEmitter::VisitAbstractConditionalOperator( // Bind the common expression if necessary. CIRGenFunction::OpaqueValueMapping binding(cgf, e); - CIRGenFunction::ConditionalEvaluation eval(cgf); - Expr *cond = e->getCond()->IgnoreParens(); mlir::Value condValue = cgf.evaluateExprAsBool(cond); + CIRGenFunction::ConditionalEvaluation eval(cgf); + return cir::TernaryOp::create( builder, loc, condValue, /*trueBuilder=*/ diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h index 3ed6151ebd44f..6fb9fd43aef6e 100644 --- a/clang/lib/CIR/CodeGen/CIRGenFunction.h +++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h @@ -2456,13 +2456,23 @@ class CIRGenFunction : public CIRGenTypeCache { /// An object to manage conditionally-evaluated expressions. class ConditionalEvaluation { CIRGenFunction &cgf; - mlir::OpBuilder::InsertPoint insertPt; + + /// The insertion point that precedes the conditional, stored as the + /// enclosing block and the operation immediately before that point. Later + /// operations (the condition, cleanup scopes) can be appended without + /// moving this point. A null \c anchorAfter means the insertion point is + /// the start of \c anchorBlock. + mlir::Block *anchorBlock; + mlir::Operation *anchorAfter = nullptr; public: ConditionalEvaluation(CIRGenFunction &cgf) - : cgf(cgf), insertPt(cgf.builder.saveInsertionPoint()) {} - ConditionalEvaluation(CIRGenFunction &cgf, mlir::OpBuilder::InsertPoint ip) - : cgf(cgf), insertPt(ip) {} + : cgf(cgf), anchorBlock(cgf.builder.getInsertionBlock()) { + assert(anchorBlock && "conditional evaluation needs an insertion point"); + mlir::Block::iterator ip = cgf.builder.getInsertionPoint(); + if (ip != anchorBlock->begin()) + anchorAfter = &*std::prev(ip); + } void beginEvaluation() { assert(cgf.outermostConditional != this); @@ -2476,10 +2486,20 @@ class CIRGenFunction : public CIRGenTypeCache { cgf.outermostConditional = nullptr; } + /// Records \p op as the last operation emitted at the pre-conditional + /// insertion point, so that a later emission lands after it rather than + /// ahead of it. + void advanceInsertPoint(mlir::Operation *op) { anchorAfter = op; } + /// Returns the insertion point which will be executed prior to each /// evaluation of the conditional code. In LLVM OG, this method /// is called getStartingBlock. - mlir::OpBuilder::InsertPoint getInsertPoint() const { return insertPt; } + mlir::OpBuilder::InsertPoint getInsertPoint() const { + if (!anchorAfter) + return mlir::OpBuilder::InsertPoint(anchorBlock, anchorBlock->begin()); + return mlir::OpBuilder::InsertPoint( + anchorAfter->getBlock(), std::next(anchorAfter->getIterator())); + } }; struct ConditionalInfo { @@ -2496,12 +2516,13 @@ class CIRGenFunction : public CIRGenTypeCache { { mlir::OpBuilder::InsertionGuard guard(builder); builder.restoreInsertionPoint(outermostConditional->getInsertPoint()); - builder.createStore( + cir::StoreOp store = builder.createStore( value.getLoc(), value, addr, /*isVolatile=*/false, /*isNontemporal=*/false, mlir::IntegerAttr::get( mlir::IntegerType::get(value.getContext(), 64), (uint64_t)addr.getAlignment().getAsAlign().value())); + outermostConditional->advanceInsertPoint(store); } } diff --git a/clang/test/CIR/CodeGen/cleanup-conditional-eh.cpp b/clang/test/CIR/CodeGen/cleanup-conditional-eh.cpp index 897dbcf78ccba..ca109028ab2f1 100644 --- a/clang/test/CIR/CodeGen/cleanup-conditional-eh.cpp +++ b/clang/test/CIR/CodeGen/cleanup-conditional-eh.cpp @@ -661,3 +661,104 @@ void test_nested_ewc(bool c1, bool c2) { // OGCG-NEXT: cleanup // OGCG: call void @_ZN1TD1Ev({{.*}} %[[REF_TMP]]) // OGCG: resume { ptr, i32 } + +struct Payload { ~Payload(); }; +Payload globalPayload; +struct Holder { Holder(); Holder(Payload); }; +struct Guard { Guard(); ~Guard(); operator bool(); }; + +// The condition of this conditional materializes its own temporary (Guard), +// whose destructor is emitted as a cir.cleanup.scope nested inside the +// full-expression scope. That scope is created after the conditional +// evaluation begins, so it is appended to the block that the Payload +// temporary's active flag must be cleared in. +// +// With exceptions enabled the flag is also read from the unwind path, so the +// clear has to dominate the landingpads as well as the normal path. Anchoring +// it ahead of the nested scope achieves both. If it were appended to the end +// of the block instead, it would sit on the normal path only, and unwinding +// out of the conditional would reach the flag load with the alloca never +// written. +void test_flag_cleared_before_cond_cleanup() { + Guard() ? Holder() : globalPayload; +} +// CIR-LABEL: @_Z37test_flag_cleared_before_cond_cleanupv +// CIR: %[[REF_TMP:.*]] = cir.alloca "ref.tmp0" {{.*}} : !cir.ptr<!rec_Guard> +// CIR: %[[ACTIVE:.*]] = cir.alloca "cleanup.cond" {{.*}} : !cir.ptr<!cir.bool> +// CIR: %[[AGG_TMP:.*]] = cir.alloca "agg.tmp0" {{.*}} : !cir.ptr<!rec_Payload> +// CIR: cir.cleanup.scope { +// The clear precedes both the Guard constructor and the nested cleanup scope. +// CIR: %[[FALSE:.*]] = cir.const #false +// CIR: cir.store %[[FALSE]], %[[ACTIVE]] : !cir.bool, !cir.ptr<!cir.bool> +// CIR: cir.call @_ZN5GuardC1Ev(%[[REF_TMP]]) +// CIR: cir.cleanup.scope { +// CIR: %[[COND:.*]] = cir.call @_ZN5GuardcvbEv(%[[REF_TMP]]) +// CIR: cir.if %[[COND]] { +// CIR: } else { +// CIR: %[[TRUE:.*]] = cir.const #true +// CIR: cir.store %[[TRUE]], %[[ACTIVE]] : !cir.bool, !cir.ptr<!cir.bool> +// CIR: } +// CIR: } cleanup all { +// CIR: cir.call @_ZN5GuardD1Ev(%[[REF_TMP]]) +// CIR: } +// CIR: } cleanup all { +// CIR: %[[IS_ACTIVE:.*]] = cir.load{{.*}} %[[ACTIVE]] +// CIR: cir.if %[[IS_ACTIVE]] { +// CIR: cir.call @_ZN7PayloadD1Ev(%[[AGG_TMP]]) +// CIR: } +// CIR: } + +// LLVM-LABEL: define dso_local void @_Z37test_flag_cleared_before_cond_cleanupv( +// LLVM: %[[REF_TMP:.*]] = alloca %struct.Guard +// LLVM: %[[ACTIVE:.*]] = alloca i8 +// LLVM: %[[AGG_TMP:.*]] = alloca %struct.Payload +// The clear dominates the invokes, so both landingpads see an initialized flag. +// LLVM: store i8 0, ptr %[[ACTIVE]] +// LLVM: invoke void @_ZN5GuardC1Ev(ptr {{.*}} %[[REF_TMP]]) +// LLVM: %[[COND:.*]] = invoke {{.*}} i1 @_ZN5GuardcvbEv(ptr {{.*}} %[[REF_TMP]]) +// LLVM: br i1 %[[COND]], label %[[TRUE_BR:.*]], label %[[FALSE_BR:.*]] +// LLVM: [[FALSE_BR]]: +// LLVM: store i8 1, ptr %[[ACTIVE]] +// Normal path. +// LLVM: call void @_ZN5GuardD1Ev(ptr {{.*}} %[[REF_TMP]]) +// LLVM: landingpad { ptr, i32 } +// LLVM: call void @_ZN5GuardD1Ev(ptr {{.*}} %[[REF_TMP]]) +// LLVM: %[[BYTE:.*]] = load i8, ptr %[[ACTIVE]] +// LLVM: %[[BOOL:.*]] = trunc i8 %[[BYTE]] to i1 +// LLVM: br i1 %[[BOOL]], label %[[DTOR:.*]], label %{{.*}} +// LLVM: [[DTOR]]: +// LLVM: call void @_ZN7PayloadD1Ev(ptr {{.*}} %[[AGG_TMP]]) +// Unwind path reads the same flag. +// LLVM: landingpad { ptr, i32 } +// LLVM: %[[EH_BYTE:.*]] = load i8, ptr %[[ACTIVE]] +// LLVM: %[[EH_BOOL:.*]] = trunc i8 %[[EH_BYTE]] to i1 +// LLVM: br i1 %[[EH_BOOL]], label %[[EH_DTOR:.*]], label %{{.*}} +// LLVM: [[EH_DTOR]]: +// LLVM: call void @_ZN7PayloadD1Ev(ptr {{.*}} %[[AGG_TMP]]) +// LLVM: resume { ptr, i32 } + +// OGCG-LABEL: define dso_local void @_Z37test_flag_cleared_before_cond_cleanupv( +// OGCG: %[[REF_TMP:.*]] = alloca %struct.Guard +// OGCG: %[[AGG_TMP:.*]] = alloca %struct.Payload +// OGCG: %[[ACTIVE:.*]] = alloca i1 +// OGCG: call void @_ZN5GuardC1Ev(ptr {{.*}} %[[REF_TMP]]) +// OGCG: store i1 false, ptr %[[ACTIVE]] +// OGCG: %[[COND:.*]] = invoke {{.*}} i1 @_ZN5GuardcvbEv(ptr {{.*}} %[[REF_TMP]]) +// OGCG: [[CONT:.*]]: +// OGCG: br i1 %[[COND]], label %[[TRUE_BR:.*]], label %[[FALSE_BR:.*]] +// OGCG: [[FALSE_BR]]: +// OGCG: store i1 true, ptr %[[ACTIVE]] +// OGCG: %[[IS_ACTIVE:.*]] = load i1, ptr %[[ACTIVE]] +// OGCG: br i1 %[[IS_ACTIVE]], label %[[DTOR:.*]], label %[[DONE:.*]] +// OGCG: [[DTOR]]: +// OGCG: call void @_ZN7PayloadD1Ev(ptr {{.*}} %[[AGG_TMP]]) +// OGCG: [[DONE]]: +// OGCG: call void @_ZN5GuardD1Ev(ptr {{.*}} %[[REF_TMP]]) +// Unwind path reads the same flag. +// OGCG: landingpad { ptr, i32 } +// OGCG: %[[EH_ACTIVE:.*]] = load i1, ptr %[[ACTIVE]] +// OGCG: br i1 %[[EH_ACTIVE]], label %[[EH_DTOR:.*]], label %{{.*}} +// OGCG: [[EH_DTOR]]: +// OGCG: call void @_ZN7PayloadD1Ev(ptr {{.*}} %[[AGG_TMP]]) +// OGCG: call void @_ZN5GuardD1Ev(ptr {{.*}} %[[REF_TMP]]) +// OGCG: resume { ptr, i32 } diff --git a/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper-eh.cpp b/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper-eh.cpp index 323ced9c2d521..c98e3acc41522 100644 --- a/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper-eh.cpp +++ b/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper-eh.cpp @@ -43,9 +43,9 @@ Wrapper makeWrapper() { // CIR: %[[CLEANUP_COND:.*]] = cir.alloca "cleanup.cond" {{.*}} : !cir.ptr<!cir.bool> // CIR: %[[AGG_TMP0:.*]] = cir.alloca "agg.tmp0" {{.*}} : !cir.ptr<!rec_std3A3Aunique_ptr3CBase3E> // CIR: cir.cleanup.scope { -// CIR: %[[FLAG:.*]] = cir.load{{.*}} %{{.*}} // CIR: %[[FALSE:.*]] = cir.const #false // CIR: cir.store %[[FALSE]], %[[CLEANUP_COND]] +// CIR: %[[FLAG:.*]] = cir.load{{.*}} %{{.*}} // CIR: cir.if %[[FLAG]] { // CIR: %[[SOURCE:.*]] = cir.call @_Z9getSourcev() // CIR: cir.call @_ZNSt10unique_ptrI4BaseEC1EPS0_(%[[AGG_TMP0]], %[[SOURCE]]) @@ -194,12 +194,12 @@ void APFixedPoint::add(int x) const { // CIR: cir.if %[[X_BOOL]] { // CIR: %[[AGG_TMP:.*]] = cir.alloca "agg.tmp.ensured" {{.*}} : !cir.ptr<!rec_APInt> // CIR: cir.cleanup.scope { -// CIR: %[[X2:.*]] = cir.load{{.*}} %[[X_ADDR]] -// CIR: %[[X2_BOOL:.*]] = cir.cast int_to_bool %[[X2]] // CIR: %[[FALSE:.*]] = cir.const #false // CIR: cir.store{{.*}} %[[FALSE]], %[[CLEANUP_COND_TRUE]] // CIR: %[[FALSE:.*]] = cir.const #false // CIR: cir.store{{.*}} %[[FALSE]], %[[CLEANUP_COND_FALSE]] +// CIR: %[[X2:.*]] = cir.load{{.*}} %[[X_ADDR]] +// CIR: %[[X2_BOOL:.*]] = cir.cast int_to_bool %[[X2]] // CIR: cir.if %[[X2_BOOL]] { // CIR: %[[TRUE:.*]] = cir.const #true // CIR: cir.store %[[TRUE]], %[[CLEANUP_COND_TRUE]] @@ -342,6 +342,16 @@ struct Entry { // (full-expr) cleanup scope, even though in the freshly emitted IR its // direct parent cleanup scope is the inner one created for the Iter // temporary. +// +// FIXME: The destruction order below is wrong, on both the normal and the +// unwind path. Iter() is constructed first and the Path temporary second, so +// reverse-of-construction order requires ~Path to run before ~Iter, as the +// OGCG checks show. CIR emits them the other way around because an +// unconditional cleanup goes on the EH stack and gets its own nested +// cir.cleanup.scope, whose cleanup region fires when the inner body ends, +// while a conditional cleanup is deferred to the enclosing full-expression +// scope and fires later. Mixing the two therefore yields push order instead +// of reverse-push order. void makeEntry() { Iter() ? Entry() : g_path; } @@ -364,6 +374,8 @@ void makeEntry() { // CIR: cir.call @_ZN5EntryC1E4Path(%[[ENSURED_F]], %[[AGG_TMP0]]) : ({{.*}}, !cir.ptr<!rec_Path> {llvm.align = 1 : i64, llvm.dereferenceable = 1 : i64, llvm.nofreeobj, llvm.noundef}) -> () // CIR: } // CIR: cir.yield +// FIXME: ~Iter runs here, when the inner scope's body ends, but it should run +// after ~Path below. // CIR: } cleanup all { // CIR: cir.call @_ZN4IterD1Ev(%[[REF_TMP]]) // CIR: cir.yield @@ -384,6 +396,7 @@ void makeEntry() { // LLVM: %[[REF_TMP:.*]] = alloca %struct.Iter // LLVM: %[[CLEANUP_COND:.*]] = alloca i8 // LLVM: %[[AGG_TMP0:.*]] = alloca %struct.Path +// LLVM: store i8 0, ptr %[[CLEANUP_COND]] // LLVM: %[[CALL:.*]] = invoke {{.*}} i1 @_ZN4ItercvbEv(ptr {{.*}} %[[REF_TMP]]) // LLVM: to label %[[CALL_CONT:.*]] unwind label %[[LPAD:.*]] // LLVM: [[CALL_CONT]]: @@ -412,7 +425,6 @@ void makeEntry() { // LLVM: call void @_ZN4IterD1Ev(ptr {{.*}} %[[REF_TMP]]) // LLVM: br label %[[EH_OUTER:.*]] // LLVM: [[NORMAL_OUTER]]: -// LLVM: store i8 0, ptr %[[CLEANUP_COND]] // LLVM: br label %[[CHECK_FLAG:.*]] // LLVM: [[CHECK_FLAG]]: // LLVM: %[[FLAG_BYTE:.*]] = load i8, ptr %[[CLEANUP_COND]] diff --git a/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper.cpp b/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper.cpp index 825172ad5b43c..d408091a73e48 100644 --- a/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper.cpp +++ b/clang/test/CIR/CodeGen/cleanup-conditional-with-wrapper.cpp @@ -43,9 +43,9 @@ Wrapper makeWrapper() { // CIR: %[[CLEANUP_COND:.*]] = cir.alloca "cleanup.cond" {{.*}} : !cir.ptr<!cir.bool> // CIR: %[[AGG_TMP0:.*]] = cir.alloca "agg.tmp0" {{.*}} : !cir.ptr<!rec_std3A3Aunique_ptr3CBase3E> // CIR: cir.cleanup.scope { -// CIR: %[[FLAG:.*]] = cir.load{{.*}} %{{.*}} // CIR: %[[FALSE:.*]] = cir.const #false // CIR: cir.store %[[FALSE]], %[[CLEANUP_COND]] +// CIR: %[[FLAG:.*]] = cir.load{{.*}} %{{.*}} // CIR: cir.if %[[FLAG]] { // CIR: %[[SOURCE:.*]] = cir.call @_Z9getSourcev() // CIR: cir.call @_ZNSt10unique_ptrI4BaseEC1EPS0_(%[[AGG_TMP0]], %[[SOURCE]]) @@ -147,12 +147,12 @@ void APFixedPoint::add(int x) const { // CIR: cir.if %[[X_BOOL]] { // CIR: %[[AGG_TMP:.*]] = cir.alloca "agg.tmp.ensured" {{.*}} : !cir.ptr<!rec_APInt> // CIR: cir.cleanup.scope { -// CIR: %[[X2:.*]] = cir.load{{.*}} %[[X_ADDR]] -// CIR: %[[X2_BOOL:.*]] = cir.cast int_to_bool %[[X2]] // CIR: %[[FALSE:.*]] = cir.const #false // CIR: cir.store{{.*}} %[[FALSE]], %[[CLEANUP_COND_TRUE]] // CIR: %[[FALSE:.*]] = cir.const #false // CIR: cir.store{{.*}} %[[FALSE]], %[[CLEANUP_COND_FALSE]] +// CIR: %[[X2:.*]] = cir.load{{.*}} %[[X_ADDR]] +// CIR: %[[X2_BOOL:.*]] = cir.cast int_to_bool %[[X2]] // CIR: cir.if %[[X2_BOOL]] { // CIR: %[[TRUE:.*]] = cir.const #true // CIR: cir.store %[[TRUE]], %[[CLEANUP_COND_TRUE]] @@ -263,6 +263,15 @@ struct Entry { // (full-expr) cleanup scope, even though in the freshly emitted IR its // direct parent cleanup scope is the inner one created for the Iter // temporary. +// +// FIXME: The destruction order below is wrong. Iter() is constructed first and +// the Path temporary second, so reverse-of-construction order requires ~Path +// to run before ~Iter, as the OGCG checks show. CIR emits them the other way +// around because an unconditional cleanup goes on the EH stack and gets its +// own nested cir.cleanup.scope, whose cleanup region fires when the inner body +// ends, while a conditional cleanup is deferred to the enclosing +// full-expression scope and fires later. Mixing the two therefore yields push +// order instead of reverse-push order. void makeEntry() { Iter() ? Entry() : g_path; } @@ -272,6 +281,8 @@ void makeEntry() { // CIR: %[[CLEANUP_COND:.*]] = cir.alloca "cleanup.cond" {{.*}} : !cir.ptr<!cir.bool> // CIR: %[[AGG_TMP0:.*]] = cir.alloca "agg.tmp0" {{.*}} : !cir.ptr<!rec_Path> // CIR: cir.cleanup.scope { +// CIR: %[[FALSE:.*]] = cir.const #false +// CIR: cir.store %[[FALSE]], %[[CLEANUP_COND]] // CIR: cir.cleanup.scope { // CIR: %[[CALL:.*]] = cir.call @_ZN4ItercvbEv(%[[REF_TMP]]) // CIR: cir.if %[[CALL]] { @@ -285,6 +296,8 @@ void makeEntry() { // CIR: cir.call @_ZN5EntryC1E4Path(%[[ENSURED_F]], %[[AGG_TMP0]]) : ({{.*}}, !cir.ptr<!rec_Path> {llvm.align = 1 : i64, llvm.dereferenceable = 1 : i64, llvm.nofreeobj, llvm.noundef}) -> () // CIR: } // CIR: cir.yield +// FIXME: ~Iter runs here, when the inner scope's body ends, but it should run +// after ~Path below. // CIR: } cleanup normal { // CIR: cir.call @_ZN4IterD1Ev(%[[REF_TMP]]) // CIR: cir.yield @@ -307,6 +320,7 @@ void makeEntry() { // LLVM: %[[AGG_TMP0:.*]] = alloca %struct.Path // LLVM: br label %[[INIT:.*]] // LLVM: [[INIT]]: +// LLVM: store i8 0, ptr %[[CLEANUP_COND]] // LLVM: %[[CALL:.*]] = call {{.*}} i1 @_ZN4ItercvbEv(ptr {{.*}} %[[REF_TMP]]) // LLVM: br i1 %[[CALL]], label %[[TRUE_BB:.*]], label %[[FALSE_BB:.*]] // LLVM: [[TRUE_BB]]: @@ -318,6 +332,8 @@ void makeEntry() { // LLVM: br label %[[COND_END]] // LLVM: [[COND_END]]: // LLVM: br label %[[AFTER_INNER:.*]] +// FIXME: ~Iter before ~Path; compare the OGCG sequence below, which destroys +// them in the correct reverse-of-construction order. // LLVM: [[AFTER_INNER]]: // LLVM: call void @_ZN4IterD1Ev(ptr {{.*}} %[[REF_TMP]]) // LLVM: br label %[[CHECK_FLAG:.*]] diff --git a/clang/test/CIR/CodeGen/cleanup-conditional.cpp b/clang/test/CIR/CodeGen/cleanup-conditional.cpp index 8516949b442be..f605e5ee6f698 100644 --- a/clang/test/CIR/CodeGen/cleanup-conditional.cpp +++ b/clang/test/CIR/CodeGen/cleanup-conditional.cpp @@ -1003,3 +1003,76 @@ void test_combined_cleanups(bool c) { // OGCG: store ptr %[[TMP_LE]], ptr %[[R]] // OGCG: call void @_ZN2LED1Ev(ptr {{.*}} %[[TMP_LE]]) // OGCG: ret void + +struct Payload { ~Payload(); }; +Payload globalPayload; +struct Holder { Holder(); Holder(Payload); }; +struct Guard { Guard(); ~Guard(); operator bool(); }; + +// The condition of this conditional materializes its own temporary (Guard), +// whose destructor is emitted as a cir.cleanup.scope nested inside the +// full-expression scope. That scope is created after the conditional +// evaluation begins, so it is appended to the block that the Payload +// temporary's active flag must be cleared in. +// +// The clear must be anchored ahead of that nested scope. If it is instead +// appended to the end of the block, it lands after the conditional and +// overwrites the "true" store in the taken arm, leaving ~Payload unreachable +// and the temporary leaked. +void test_flag_cleared_before_cond_cleanup() { + Guard() ? Holder() : globalPayload; +} +// CIR-LABEL: @_Z37test_flag_cleared_before_cond_cleanupv +// CIR: %[[REF_TMP:.*]] = cir.alloca "ref.tmp0" {{.*}} : !cir.ptr<!rec_Guard> +// CIR: %[[ACTIVE:.*]] = cir.alloca "cleanup.cond" {{.*}} : !cir.ptr<!cir.bool> +// CIR: %[[AGG_TMP:.*]] = cir.alloca "agg.tmp0" {{.*}} : !cir.ptr<!rec_Payload> +// CIR: cir.cleanup.scope { +// The clear precedes both the Guard constructor and the nested cleanup scope. +// CIR: %[[FALSE:.*]] = cir.const #false +// CIR: cir.store %[[FALSE]], %[[ACTIVE]] : !cir.bool, !cir.ptr<!cir.bool> +// CIR: cir.call @_ZN5GuardC1Ev(%[[REF_TMP]]) +// CIR: cir.cleanup.scope { +// CIR: %[[COND:.*]] = cir.call @_ZN5GuardcvbEv(%[[REF_TMP]]) +// CIR: cir.if %[[COND]] { +// CIR: cir.call @_ZN6HolderC1Ev(%{{.*}}) +// CIR: } else { +// CIR: %[[TRUE:.*]] = cir.const #true +// CIR: cir.store %[[TRUE]], %[[ACTIVE]] : !cir.bool, !cir.ptr<!cir.bool> +// CIR: cir.call @_ZN6HolderC1E7Payload(%{{.*}}, %[[AGG_TMP]]) +// CIR: } +// CIR: } cleanup normal { +// CIR: cir.call @_ZN5GuardD1Ev(%[[REF_TMP]]) +// CIR: } +// CIR: } cleanup normal { +// CIR: %[[IS_ACTIVE:.*]] = cir.load{{.*}} %[[ACTIVE]] +// CIR: cir.if %[[IS_ACTIVE]] { +// CIR: cir.call @_ZN7PayloadD1Ev(%[[AGG_TMP]]) +// CIR: } +// CIR: } + +// LLVM-LABEL: define dso_local void @_Z37test_flag_cleared_before_cond_cleanupv( +// LLVM: %[[REF_TMP:.*]] = alloca %struct.Guard +// LLVMCIR: %[[ACTIVE:.*]] = alloca i8 +// LLVM: %[[AGG_TMP:.*]] = alloca %struct.Payload +// OGCG: %[[ACTIVE:.*]] = alloca i1 +// LLVMCIR: store i8 0, ptr %[[ACTIVE]] +// LLVM: call void @_ZN5GuardC1Ev(ptr {{.*}} %[[REF_TMP]]) +// LLVM: %[[COND:.*]] = call {{.*}} i1 @_ZN5GuardcvbEv(ptr {{.*}} %[[REF_TMP]]) +// OGCG: store i1 false, ptr %[[ACTIVE]] +// LLVM: br i1 %[[COND]], label %[[TRUE_BR:.*]], label %[[FALSE_BR:.*]] +// LLVM: [[FALSE_BR]]: +// LLVMCIR: store i8 1, ptr %[[ACTIVE]] +// FIXME: CIR destroys Guard before Payload; reverse-of-construction order +// requires ~Payload to run first, as OGCG below does. Tracked separately from +// the active-flag placement this test covers. +// LLVMCIR: call void @_ZN5GuardD1Ev(ptr {{.*}} %[[REF_TMP]]) +// LLVMCIR: %[[ACTIVE_BYTE:.*]] = load i8, ptr %[[ACTIVE]] +// LLVMCIR: %[[ACTIVE_BOOL:.*]] = trunc i8 %[[ACTIVE_BYTE]] to i1 +// LLVMCIR: br i1 %[[ACTIVE_BOOL]], label %[[DTOR:.*]], label %[[SKIP:.*]] +// OGCG: store i1 true, ptr %[[ACTIVE]] +// OGCG: %[[IS_ACTIVE:.*]] = load i1, ptr %[[ACTIVE]] +// OGCG: br i1 %[[IS_ACTIVE]], label %[[DTOR:.*]], label %[[DONE:.*]] +// LLVM: [[DTOR]]: +// LLVM: call void @_ZN7PayloadD1Ev(ptr {{.*}} %[[AGG_TMP]]) +// OGCG: [[DONE]]: +// OGCG: call void @_ZN5GuardD1Ev(ptr {{.*}} %[[REF_TMP]]) _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
