Skip to content

Commit 0d98eb6

Browse files
andykaylorandykaylor
authored
[CIR] Fix ordering of lifetime-extended cleanups (#200874)
We had a bug that was causing any lifetime-extended cleanups that occurred within a full-expression cleanup scope to be emitted prematurely when the expression also required deferred conditional cleanups. This was, in some cases, causing a dangling reference to temporaries that had already been destructed. Luckily, it was also causing us to not emit a return at the end of the function in one case, leading the verifier to draw attention to this problem. This change introduces new functions in RunCleanupsScope to allow "ordinary" EH stack cleanups to be force-emitted separately from lifetime-extended cleanups. Classic codegen doesn't need this capability because it handles deferred conditional cleanups very differently than CIR due to its flat/branching approach. The testing for this fix did uncover a significant issue wherein CIR is calling destructors in the wrong order even after the fix in this PR. However, that's a pre-existing issue that will require changes beyond the scope of this fix, so I'll handle it in a follow-up. Assisted-by: Cursor / claude-opus-4.7
1 parent dc448da commit 0d98eb6

4 files changed

Lines changed: 194 additions & 6 deletions

File tree

‎clang/lib/CIR/CodeGen/CIRGenCleanup.cpp‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,9 +198,10 @@ void CIRGenFunction::FullExprCleanupScope::exit(
198198
cgf.builder.createStore(val.getLoc(), val, temp);
199199
}
200200

201-
// Pop any EH and lifetime-extended cleanups that were pushed during
202-
// the expression (e.g. temporary destructors).
203-
cleanups.forceCleanup();
201+
// Pop any EH cleanups that were pushed during the expression but leave
202+
// any lifetime-extended cleanups so that they can be promoted to the EH
203+
// stack after we've finished emitting any deferred cleanups.
204+
cleanups.forceCleanupExceptLifetimeExtended();
204205

205206
// Make sure the cleanup scope body region has a terminator.
206207
{
@@ -265,6 +266,12 @@ void CIRGenFunction::FullExprCleanupScope::exit(
265266
cgf.deferredConditionalCleanupStack.truncate(oldSize);
266267
cgf.builder.setInsertionPointAfter(scope);
267268

269+
// Promote any lifetime-extended cleanups onto the EH scope stack. The new
270+
// cir.cleanup.scope ops created here will wrap any code in the enclosing
271+
// scope, including reloads of any spilled values below, so the
272+
// lifetime-extended destructors run at the correct point.
273+
cleanups.forceLifetimeExtendedCleanups();
274+
268275
// Reload spilled values now that the builder is after the closed scope.
269276
for (auto [addr, valPtr] : llvm::zip(tempAllocas, valuesToReload)) {
270277
if (!addr.isValid())

‎clang/lib/CIR/CodeGen/CIRGenFunction.h‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,6 +1205,29 @@ class CIRGenFunction : public CIRGenTypeCache {
12051205
cgf.currentCleanupStackDepth = oldCleanupStackDepth;
12061206
}
12071207

1208+
/// Force the emission of EH cleanups now, but defer promoting any
1209+
/// lifetime-extended cleanup entries onto the EH scope stack. The caller
1210+
/// must subsequently call forceLifetimeExtendedCleanups() to finalize the
1211+
/// scope.
1212+
void forceCleanupExceptLifetimeExtended() {
1213+
assert(performCleanup && "Already forced cleanup");
1214+
cgf.didCallStackSave = oldDidCallStackSave;
1215+
deactivateCleanups.forceDeactivate();
1216+
cgf.popCleanupBlocks(cleanupStackDepth);
1217+
}
1218+
1219+
/// Promote any pending lifetime-extended cleanup entries onto the EH scope
1220+
/// stack at the current insertion point and finalize this scope. This must
1221+
/// be paired with a prior call to forceCleanupExceptLifetimeExtended().
1222+
void forceLifetimeExtendedCleanups() {
1223+
assert(performCleanup && "Already forced cleanup");
1224+
assert(deactivateCleanups.deactivated &&
1225+
"forceCleanupExceptLifetimeExtended() must be called first");
1226+
cgf.popCleanupBlocks(cleanupStackDepth, lifetimeExtendedCleanupStackSize);
1227+
performCleanup = false;
1228+
cgf.currentCleanupStackDepth = oldCleanupStackDepth;
1229+
}
1230+
12081231
/// Whether there are any pending cleanups that have been pushed since
12091232
/// this scope was entered.
12101233
bool hasPendingCleanups() const {

‎clang/test/CIR/CodeGen/cleanup-conditional.cpp‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -874,3 +874,161 @@ _Complex float test_complex_cond_cleanup(bool b, _Complex float x) {
874874
// OGCG: [[DTOR]]:
875875
// OGCG: call void @_ZN5CplxDD1Ev(ptr {{.*}} %[[TMP]])
876876
// OGCG: br label %[[DONE]]
877+
878+
struct LE {
879+
LE(int);
880+
~LE();
881+
};
882+
883+
void test_lifetime_ext_cond_ref(bool c) {
884+
const LE &r = c ? LE(1) : LE(2);
885+
}
886+
// CIR-LABEL: @_Z26test_lifetime_ext_cond_refb
887+
// CIR: %[[TMP:.*]] = cir.alloca !rec_LE, !cir.ptr<!rec_LE>, ["ref.tmp0"]
888+
// CIR: %[[R:.*]] = cir.alloca !cir.ptr<!rec_LE>, !cir.ptr<!cir.ptr<!rec_LE>>, ["r", init, const]
889+
// CIR: %[[SPILL:.*]] = cir.alloca !cir.ptr<!rec_LE>, !cir.ptr<!cir.ptr<!rec_LE>>, ["tmp.exprcleanup"]
890+
// CIR: cir.if %{{.*}} {
891+
// CIR: cir.call @_ZN2LEC1Ei(%[[TMP]], %{{.*}})
892+
// CIR: } else {
893+
// CIR: cir.call @_ZN2LEC1Ei(%[[TMP]], %{{.*}})
894+
// CIR: }
895+
// CIR: cir.store {{.*}} %[[TMP]], %[[SPILL]]
896+
// CIR: cir.cleanup.scope {
897+
// CIR: %[[RELOAD:.*]] = cir.load {{.*}} %[[SPILL]] : !cir.ptr<!cir.ptr<!rec_LE>>, !cir.ptr<!rec_LE>
898+
// CIR: cir.store {{.*}} %[[RELOAD]], %[[R]]
899+
// CIR: cir.yield
900+
// CIR: } cleanup normal {
901+
// CIR: cir.call @_ZN2LED1Ev(%[[TMP]]) nothrow
902+
// CIR: cir.yield
903+
// CIR: }
904+
// CIR: cir.return
905+
906+
// LLVM-LABEL: define dso_local void @_Z26test_lifetime_ext_cond_refb(
907+
// LLVM: %[[TMP:.*]] = alloca %struct.LE
908+
// LLVM: %[[R:.*]] = alloca ptr
909+
// LLVM: %[[SPILL:.*]] = alloca ptr
910+
// LLVM: br i1 %{{.*}}, label %[[TRUE:.*]], label %[[FALSE:.*]]
911+
// LLVM: [[TRUE]]:
912+
// LLVM: call void @_ZN2LEC1Ei(ptr {{.*}} %[[TMP]], i32 {{.*}} 1)
913+
// LLVM: [[FALSE]]:
914+
// LLVM: call void @_ZN2LEC1Ei(ptr {{.*}} %[[TMP]], i32 {{.*}} 2)
915+
// LLVM: store ptr %[[TMP]], ptr %[[SPILL]]
916+
// LLVM: %[[RELOAD:.*]] = load ptr, ptr %[[SPILL]]
917+
// LLVM: store ptr %[[RELOAD]], ptr %[[R]]
918+
// LLVM: call void @_ZN2LED1Ev(ptr {{.*}} %[[TMP]])
919+
// LLVM: ret void
920+
921+
// OGCG-LABEL: define dso_local void @_Z26test_lifetime_ext_cond_refb(
922+
// OGCG: %[[R:.*]] = alloca ptr
923+
// OGCG: %[[TMP:.*]] = alloca %struct.LE
924+
// OGCG: br i1 %{{.*}}, label %[[TRUE:.*]], label %[[FALSE:.*]]
925+
// OGCG: [[TRUE]]:
926+
// OGCG: call void @_ZN2LEC1Ei(ptr {{.*}} %[[TMP]], i32 {{.*}} 1)
927+
// OGCG: [[FALSE]]:
928+
// OGCG: call void @_ZN2LEC1Ei(ptr {{.*}} %[[TMP]], i32 {{.*}} 2)
929+
// OGCG: store ptr %[[TMP]], ptr %[[R]]
930+
// OGCG: call void @_ZN2LED1Ev(ptr {{.*}} %[[TMP]])
931+
// OGCG: ret void
932+
933+
void test_combined_cleanups(bool c) {
934+
const LE &r = LE((S().get(), c ? B().get() : 0));
935+
}
936+
// CIR-LABEL: @_Z22test_combined_cleanupsb
937+
// CIR: %[[TMP_LE:.*]] = cir.alloca !rec_LE, !cir.ptr<!rec_LE>, ["ref.tmp0"]
938+
// CIR: %[[R:.*]] = cir.alloca !cir.ptr<!rec_LE>, !cir.ptr<!cir.ptr<!rec_LE>>, ["r", init, const]
939+
// CIR: %[[TMP_S:.*]] = cir.alloca !rec_S, !cir.ptr<!rec_S>, ["ref.tmp1"]
940+
// CIR: %[[TMP_B:.*]] = cir.alloca !rec_B, !cir.ptr<!rec_B>, ["ref.tmp2"]
941+
// CIR: %[[ACT_B:.*]] = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["cleanup.cond"]
942+
// CIR: %[[SPILL:.*]] = cir.alloca !cir.ptr<!rec_LE>, !cir.ptr<!cir.ptr<!rec_LE>>, ["tmp.exprcleanup"]
943+
// CIR: cir.cleanup.scope {
944+
// CIR: cir.call @_ZN1SC1Ev(%[[TMP_S]])
945+
// CIR: cir.cleanup.scope {
946+
// CIR: cir.call @_ZN1S3getEv(%[[TMP_S]])
947+
// CIR: cir.store {{.*}}, %[[ACT_B]]
948+
// CIR: %{{.*}} = cir.ternary({{.*}}, true {
949+
// CIR: cir.call @_ZN1BC1Ev(%[[TMP_B]])
950+
// CIR: cir.store {{.*}}, %[[ACT_B]]
951+
// CIR: cir.call @_ZN1B3getEv(%[[TMP_B]])
952+
// CIR: }, false {
953+
// CIR: })
954+
// CIR: cir.call @_ZN2LEC1Ei(%[[TMP_LE]], %{{.*}})
955+
// CIR: cir.store {{.*}} %[[TMP_LE]], %[[SPILL]]
956+
// CIR: cir.yield
957+
// CIR: } cleanup normal {
958+
// CIR: cir.call @_ZN1SD1Ev(%[[TMP_S]]) nothrow
959+
// CIR: cir.yield
960+
// CIR: }
961+
// CIR: cir.yield
962+
// CIR: } cleanup normal {
963+
// CIR: %[[FLAG:.*]] = cir.load {{.*}} %[[ACT_B]]
964+
// CIR: cir.if %[[FLAG]] {
965+
// CIR: cir.call @_ZN1BD1Ev(%[[TMP_B]]) nothrow
966+
// CIR: }
967+
// CIR: cir.yield
968+
// CIR: }
969+
// CIR: cir.cleanup.scope {
970+
// CIR: %[[RELOAD:.*]] = cir.load {{.*}} %[[SPILL]]
971+
// CIR: cir.store {{.*}} %[[RELOAD]], %[[R]]
972+
// CIR: cir.yield
973+
// CIR: } cleanup normal {
974+
// CIR: cir.call @_ZN2LED1Ev(%[[TMP_LE]]) nothrow
975+
// CIR: cir.yield
976+
// CIR: }
977+
// CIR: cir.return
978+
979+
// LLVM-LABEL: define dso_local void @_Z22test_combined_cleanupsb(
980+
// LLVM: %[[TMP_LE:.*]] = alloca %struct.LE
981+
// LLVM: %[[R:.*]] = alloca ptr
982+
// LLVM: %[[TMP_S:.*]] = alloca %struct.S
983+
// LLVM: %[[TMP_B:.*]] = alloca %struct.B
984+
// LLVM: %[[ACT_B:.*]] = alloca i8
985+
// LLVM: %[[SPILL:.*]] = alloca ptr
986+
// LLVM: call void @_ZN1SC1Ev(ptr {{.*}} %[[TMP_S]])
987+
// LLVM: call {{.*}} i32 @_ZN1S3getEv(ptr {{.*}} %[[TMP_S]])
988+
// LLVM: store i8 0, ptr %[[ACT_B]]
989+
// LLVM: br i1 %{{.*}}, label %[[T:.*]], label %[[F:.*]]
990+
// LLVM: [[T]]:
991+
// LLVM: call void @_ZN1BC1Ev(ptr {{.*}} %[[TMP_B]])
992+
// LLVM: store i8 1, ptr %[[ACT_B]]
993+
// LLVM: call {{.*}} i32 @_ZN1B3getEv(ptr {{.*}} %[[TMP_B]])
994+
// LLVM: [[F]]:
995+
// LLVM: phi i32 [ 0, %[[F]] ], [ %{{.*}}, %[[T]] ]
996+
// LLVM: call void @_ZN2LEC1Ei(ptr {{.*}} %[[TMP_LE]], i32 {{.*}})
997+
// LLVM: store ptr %[[TMP_LE]], ptr %[[SPILL]]
998+
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[TMP_S]])
999+
// LLVM: %[[FLAG_BYTE:.*]] = load i8, ptr %[[ACT_B]]
1000+
// LLVM: %[[FLAG:.*]] = trunc i8 %[[FLAG_BYTE]] to i1
1001+
// LLVM: br i1 %[[FLAG]], label %[[B_DTOR:.*]], label %[[B_DONE:.*]]
1002+
// LLVM: [[B_DTOR]]:
1003+
// LLVM: call void @_ZN1BD1Ev(ptr {{.*}} %[[TMP_B]])
1004+
// LLVM: [[B_DONE]]:
1005+
// LLVM: %[[RELOAD:.*]] = load ptr, ptr %[[SPILL]]
1006+
// LLVM: store ptr %[[RELOAD]], ptr %[[R]]
1007+
// LLVM: call void @_ZN2LED1Ev(ptr {{.*}} %[[TMP_LE]])
1008+
// LLVM: ret void
1009+
1010+
// OGCG-LABEL: define dso_local void @_Z22test_combined_cleanupsb(
1011+
// OGCG: %[[R:.*]] = alloca ptr
1012+
// OGCG: %[[TMP_LE:.*]] = alloca %struct.LE
1013+
// OGCG: %[[TMP_S:.*]] = alloca %struct.S
1014+
// OGCG: %[[TMP_B:.*]] = alloca %struct.B
1015+
// OGCG: %[[ACT_B:.*]] = alloca i1
1016+
// OGCG: call void @_ZN1SC1Ev(ptr {{.*}} %[[TMP_S]])
1017+
// OGCG: call {{.*}} i32 @_ZN1S3getEv(ptr {{.*}} %[[TMP_S]])
1018+
// OGCG: store i1 false, ptr %[[ACT_B]]
1019+
// OGCG: br i1 %{{.*}}, label %[[T:.*]], label %[[F:.*]]
1020+
// OGCG: [[T]]:
1021+
// OGCG: call void @_ZN1BC1Ev(ptr {{.*}} %[[TMP_B]])
1022+
// OGCG: store i1 true, ptr %[[ACT_B]]
1023+
// OGCG: call {{.*}} i32 @_ZN1B3getEv(ptr {{.*}} %[[TMP_B]])
1024+
// OGCG: [[F]]:
1025+
// OGCG: phi i32
1026+
// OGCG: call void @_ZN2LEC1Ei(ptr {{.*}} %[[TMP_LE]], i32 {{.*}})
1027+
// OGCG: br i1 %{{.*}}, label %[[B_DTOR:.*]], label %[[B_DONE:.*]]
1028+
// OGCG: [[B_DTOR]]:
1029+
// OGCG: call void @_ZN1BD1Ev(ptr {{.*}} %[[TMP_B]])
1030+
// OGCG: [[B_DONE]]:
1031+
// OGCG: call void @_ZN1SD1Ev(ptr {{.*}} %[[TMP_S]])
1032+
// OGCG: store ptr %[[TMP_LE]], ptr %[[R]]
1033+
// OGCG: call void @_ZN2LED1Ev(ptr {{.*}} %[[TMP_LE]])
1034+
// OGCG: ret void

‎clang/test/CIR/CodeGen/loop-cond-cleanup.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -279,9 +279,9 @@ void while_body_temp_ref() {
279279
// LLVM: [[FALSE]]:
280280
// LLVM: call void @_ZN1SC1Ev(ptr {{.*}} %[[REF_TMP]])
281281
// LLVM: store ptr %[[REF_TMP]], ptr %[[SPILL]]
282-
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[REF_TMP]])
283282
// LLVM: %[[RELOAD:.*]] = load ptr, ptr %[[SPILL]]
284283
// LLVM: store ptr %[[RELOAD]], ptr %[[OP]]
284+
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[REF_TMP]])
285285
// LLVM: br label %[[LOOP_COND]]
286286
// LLVM: [[AFTER]]:
287287
// LLVM: ret void
@@ -346,9 +346,9 @@ void for_body_temp_ref() {
346346
// LLVM: [[FALSE]]:
347347
// LLVM: call void @_ZN1SC1Ev(ptr {{.*}} %[[REF_TMP]])
348348
// LLVM: store ptr %[[REF_TMP]], ptr %[[SPILL]]
349-
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[REF_TMP]])
350349
// LLVM: %[[RELOAD:.*]] = load ptr, ptr %[[SPILL]]
351350
// LLVM: store ptr %[[RELOAD]], ptr %[[OP]]
351+
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[REF_TMP]])
352352
// LLVM: %[[OLDI:.*]] = load i32, ptr %[[I]]
353353
// LLVM: %[[NEWI:.*]] = add nsw i32 %[[OLDI]], 1
354354
// LLVM: store i32 %[[NEWI]], ptr %[[I]]
@@ -421,9 +421,9 @@ void do_body_temp_ref() {
421421
// LLVM: [[FALSE]]:
422422
// LLVM: call void @_ZN1SC1Ev(ptr {{.*}} %[[REF_TMP]])
423423
// LLVM: store ptr %[[REF_TMP]], ptr %[[SPILL]]
424-
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[REF_TMP]])
425424
// LLVM: %[[RELOAD:.*]] = load ptr, ptr %[[SPILL]]
426425
// LLVM: store ptr %[[RELOAD]], ptr %[[OP]]
426+
// LLVM: call void @_ZN1SD1Ev(ptr {{.*}} %[[REF_TMP]])
427427
// LLVM: br label %[[LOOP_COND]]
428428
// LLVM: [[AFTER]]:
429429
// LLVM: ret void

0 commit comments

Comments
 (0)