[CodeGen] Fix cleanup attribute for C89 for-loop init variables #156643
+86
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In C89, for-init variables have function scope, so cleanup should occur at function exit, not loop exit. This implements deferred cleanup registration for C89 mode while preserving C99+ behavior. Fixes llvm#154624
|
I've implemented the change using a deferred cleanup approach. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
"End of the function" is not really correct. It's still the end of the scope... just the scope in question is different. This matters if you have, for example, a nested loop.
Probably need to change CodeGenFunction::EmitForStmt to push the scope at the right point.
Move processDeferredFunctionCleanups() to EmitForStmt to ensure cleanup happens at correct scope boundary for C89 for-loop init variables.
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Jongmyeong Choi (jongmyeong-choi) ChangesIn C89, for-init variables have function scope, so cleanup should occur at function exit, not loop exit. This implements deferred cleanup registration for C89 mode while preserving C99+ behavior. Fixes #154624 Full diff: https://github.com/llvm/llvm-project/pull/156643.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 8a1675848e13c..193d82d6b1486 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -2230,9 +2230,18 @@ void CodeGenFunction::EmitAutoVarCleanups(const AutoVarEmission &emission) {
llvm::Constant *F = CGM.GetAddrOfFunction(FD);
assert(F && "Could not find function!");
- const CGFunctionInfo &Info = CGM.getTypes().arrangeFunctionDeclaration(FD);
- EHStack.pushCleanup<CallCleanupFunction>(NormalAndEHCleanup, F, &Info, &D,
- CA);
+ // Check if we're in C89 mode and should defer cleanup to function scope
+ bool isC89Mode = !getLangOpts().C99 && !getLangOpts().CPlusPlus;
+ if (isC89Mode) {
+ const CGFunctionInfo &Info =
+ CGM.getTypes().arrangeFunctionDeclaration(FD);
+ addDeferredFunctionCleanup(F, &Info, &D, CA);
+ } else {
+ const CGFunctionInfo &Info =
+ CGM.getTypes().arrangeFunctionDeclaration(FD);
+ EHStack.pushCleanup<CallCleanupFunction>(NormalAndEHCleanup, F, &Info, &D,
+ CA);
+ }
}
// If this is a block variable, call _Block_object_destroy
@@ -2963,3 +2972,20 @@ CodeGenModule::getOMPAllocateAlignment(const VarDecl *VD) {
}
return std::nullopt;
}
+
+void CodeGenFunction::addDeferredFunctionCleanup(llvm::Constant *F,
+ const CGFunctionInfo *Info,
+ const VarDecl *Var,
+ const CleanupAttr *Attribute) {
+ DeferredFunctionCleanups.emplace_back(F, Info, Var, Attribute);
+}
+
+void CodeGenFunction::processDeferredFunctionCleanups() {
+ // Process all deferred cleanups at function exit
+ for (const auto &cleanup : DeferredFunctionCleanups) {
+ EHStack.pushCleanup<CallCleanupFunction>(NormalAndEHCleanup,
+ cleanup.CleanupFn, cleanup.FnInfo,
+ cleanup.Var, cleanup.Attribute);
+ }
+ DeferredFunctionCleanups.clear();
+}
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 031ef73214e76..f30d6601a58cb 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -1421,6 +1421,9 @@ void CodeGenFunction::EmitForStmt(const ForStmt &S,
ForScope.ForceCleanup();
+ // Process deferred function cleanups before checking for regular cleanups
+ processDeferredFunctionCleanups();
+
LoopStack.pop();
// Emit the fall-through block.
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 652fe672f15e3..d3455d13eb21b 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -405,6 +405,7 @@ void CodeGenFunction::FinishFunction(SourceLocation EndLoc) {
// parameters. Do this in whatever block we're currently in; it's
// important to do this before we enter the return block or return
// edges will be *really* confused.
+
bool HasCleanups = EHStack.stable_begin() != PrologueCleanupDepth;
bool HasOnlyNoopCleanups =
HasCleanups && EHStack.containsOnlyNoopCleanups(PrologueCleanupDepth);
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index fc65199a0f154..2fb7a44d3ae10 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -392,6 +392,23 @@ class CodeGenFunction : public CodeGenTypeCache {
/// cleanups associated with the parameters.
EHScopeStack::stable_iterator PrologueCleanupDepth;
+ /// Structure for deferred function-level cleanups
+ /// (e.g., C89 for-init cleanup variables)
+ struct DeferredCleanupInfo {
+ llvm::Constant *CleanupFn;
+ const CGFunctionInfo *FnInfo;
+ const VarDecl *Var;
+ const CleanupAttr *Attribute;
+
+ DeferredCleanupInfo(llvm::Constant *F, const CGFunctionInfo *Info,
+ const VarDecl *V, const CleanupAttr *A)
+ : CleanupFn(F), FnInfo(Info), Var(V), Attribute(A) {}
+ };
+
+ /// List of cleanups that should be registered at function exit instead of
+ /// current scope
+ SmallVector<DeferredCleanupInfo, 4> DeferredFunctionCleanups;
+
/// ReturnBlock - Unified return block.
JumpDest ReturnBlock;
@@ -3471,6 +3488,16 @@ class CodeGenFunction : public CodeGenTypeCache {
void emitAutoVarTypeCleanup(const AutoVarEmission &emission,
QualType::DestructionKind dtorKind);
+ /// Add a cleanup to be deferred until function exit
+ /// (for C89 for-init variables)
+ void addDeferredFunctionCleanup(llvm::Constant *CleanupFn,
+ const CGFunctionInfo *FnInfo,
+ const VarDecl *Var,
+ const CleanupAttr *Attribute);
+
+ /// Process all deferred function-level cleanups
+ void processDeferredFunctionCleanups();
+
void MaybeEmitDeferredVarDeclInit(const VarDecl *var);
/// Emits the alloca and debug information for the size expressions for each
diff --git a/clang/test/CodeGen/attr-cleanup.c b/clang/test/CodeGen/attr-cleanup.c
index 755ede86c1382..c0d9e97927446 100644
--- a/clang/test/CodeGen/attr-cleanup.c
+++ b/clang/test/CodeGen/attr-cleanup.c
@@ -5,3 +5,29 @@ void g(void) {
__attribute__((cleanup(f))) void *g;
}
+// Test for cleanup in for-loop initialization (PR #154624)
+// RUN: %clang_cc1 -std=c89 -emit-llvm %s -o - | FileCheck %s --check-prefix=C89
+// RUN: %clang_cc1 -std=c99 -emit-llvm %s -o - | FileCheck %s --check-prefix=C99
+
+void cleaner(int *p);
+
+// C89-LABEL: define{{.*}} void @test_nested_for_loop_cleanup()
+// C99-LABEL: define{{.*}} void @test_nested_for_loop_cleanup()
+void test_nested_for_loop_cleanup(void) {
+ for (int i = 10; 0;) {
+ for (__attribute__((cleanup(cleaner))) int j = 20; 0;)
+ ;
+
+#ifndef __STDC_VERSION__
+ if (j > 15) {
+ // do something with inner variable
+ }
+#endif
+ }
+}
+
+// C89: if.end:
+// C89-NEXT: call void @cleaner(ptr noundef %j)
+
+// C99: for.cond.cleanup{{[0-9]*}}:
+// C99-NEXT: call void @cleaner(ptr noundef %j)
|
There was a problem hiding this comment.
Instead of creating a separate cleanup stack and adding special deferred handling for that cleanup stack, can we just skip creating the scope in the first place? We have LexicalScope ForScope(*this, S.getSourceRange());; can we replace that with something like the following?
std::optional<LexicalScope> ForScope;
if (getLangOpts().C99 || getLangOpts().CPlusPlus)
ForScope.emplace(*this, S.getSourceRange());
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In C89, for-init variables have function scope, so cleanup should occur at function exit, not loop exit. This implements deferred cleanup registration for C89 mode while preserving C99+ behavior.
Fixes #154624