[Clang] Fix segmentation fault caused by VarBypassDetector stack overflow on deeply nested expressions
#124128
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
…flow on deeply nested expressions This happens when using -O2. Added a test that reproduces it based on a test that was reverted in llvm#111701: https://github.com/bricknerb/llvm-project/blob/93e4a7386ec897e53d7330c6206d38759a858be2/clang/test/CodeGen/deeply-nested-expressions.cpp However, this test is slow and likely to be hard to maintained as discussed in https://github.com/llvm/llvm-project/pull/111701/files/1a63281b6c240352653fd2e4299755c1f32a76f4#r1795518779 so unless there are objections I plan to remove this test before merging.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Boaz Brickner (bricknerb) ChangesThis happens when using Added a test that reproduces it based on a test that was reverted in #111701: https://github.com/bricknerb/llvm-project/blob/93e4a7386ec897e53d7330c6206d38759a858be2/clang/test/CodeGen/deeply-nested-expressions.cpp However, this test is slow and likely to be hard to maintained as discussed in https://github.com/llvm/llvm-project/pull/111701/files/1a63281b6c240352653fd2e4299755c1f32a76f4#r1795518779 so unless there are objections I plan to remove this test before merging. Full diff: https://github.com/llvm/llvm-project/pull/124128.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 11fdddba1144bb..6bdfbbd4013039 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1533,7 +1533,7 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
// Initialize helper which will detect jumps which can cause invalid
// lifetime markers.
if (ShouldEmitLifetimeMarkers)
- Bypasses.Init(Body);
+ Bypasses.Init(CGM, Body);
}
// Emit the standard function prologue.
diff --git a/clang/lib/CodeGen/VarBypassDetector.cpp b/clang/lib/CodeGen/VarBypassDetector.cpp
index 6eda83dfdef2f3..7b2b3542928ad7 100644
--- a/clang/lib/CodeGen/VarBypassDetector.cpp
+++ b/clang/lib/CodeGen/VarBypassDetector.cpp
@@ -8,6 +8,7 @@
#include "VarBypassDetector.h"
+#include "CodeGenModule.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
#include "clang/AST/Stmt.h"
@@ -17,13 +18,13 @@ using namespace CodeGen;
/// Clear the object and pre-process for the given statement, usually function
/// body statement.
-void VarBypassDetector::Init(const Stmt *Body) {
+void VarBypassDetector::Init(CodeGenModule &CGM, const Stmt *Body) {
FromScopes.clear();
ToScopes.clear();
Bypasses.clear();
Scopes = {{~0U, nullptr}};
unsigned ParentScope = 0;
- AlwaysBypassed = !BuildScopeInformation(Body, ParentScope);
+ AlwaysBypassed = !BuildScopeInformation(CGM, Body, ParentScope);
if (!AlwaysBypassed)
Detect();
}
@@ -31,7 +32,7 @@ void VarBypassDetector::Init(const Stmt *Body) {
/// Build scope information for a declaration that is part of a DeclStmt.
/// Returns false if we failed to build scope information and can't tell for
/// which vars are being bypassed.
-bool VarBypassDetector::BuildScopeInformation(const Decl *D,
+bool VarBypassDetector::BuildScopeInformation(CodeGenModule &CGM, const Decl *D,
unsigned &ParentScope) {
const VarDecl *VD = dyn_cast<VarDecl>(D);
if (VD && VD->hasLocalStorage()) {
@@ -41,7 +42,7 @@ bool VarBypassDetector::BuildScopeInformation(const Decl *D,
if (const VarDecl *VD = dyn_cast<VarDecl>(D))
if (const Expr *Init = VD->getInit())
- return BuildScopeInformation(Init, ParentScope);
+ return BuildScopeInformation(CGM, Init, ParentScope);
return true;
}
@@ -50,7 +51,7 @@ bool VarBypassDetector::BuildScopeInformation(const Decl *D,
/// LabelAndGotoScopes and recursively walking the AST as needed.
/// Returns false if we failed to build scope information and can't tell for
/// which vars are being bypassed.
-bool VarBypassDetector::BuildScopeInformation(const Stmt *S,
+bool VarBypassDetector::BuildScopeInformation(CodeGenModule &CGM, const Stmt *S,
unsigned &origParentScope) {
// If this is a statement, rather than an expression, scopes within it don't
// propagate out into the enclosing scope. Otherwise we have to worry about
@@ -68,12 +69,12 @@ bool VarBypassDetector::BuildScopeInformation(const Stmt *S,
case Stmt::SwitchStmtClass:
if (const Stmt *Init = cast<SwitchStmt>(S)->getInit()) {
- if (!BuildScopeInformation(Init, ParentScope))
+ if (!BuildScopeInformation(CGM, Init, ParentScope))
return false;
++StmtsToSkip;
}
if (const VarDecl *Var = cast<SwitchStmt>(S)->getConditionVariable()) {
- if (!BuildScopeInformation(Var, ParentScope))
+ if (!BuildScopeInformation(CGM, Var, ParentScope))
return false;
++StmtsToSkip;
}
@@ -86,7 +87,7 @@ bool VarBypassDetector::BuildScopeInformation(const Stmt *S,
case Stmt::DeclStmtClass: {
const DeclStmt *DS = cast<DeclStmt>(S);
for (auto *I : DS->decls())
- if (!BuildScopeInformation(I, origParentScope))
+ if (!BuildScopeInformation(CGM, I, origParentScope))
return false;
return true;
}
@@ -126,7 +127,11 @@ bool VarBypassDetector::BuildScopeInformation(const Stmt *S,
}
// Recursively walk the AST.
- if (!BuildScopeInformation(SubStmt, ParentScope))
+ bool Result;
+ CGM.runWithSufficientStackSpace(S->getEndLoc(), [&] {
+ Result = BuildScopeInformation(CGM, SubStmt, ParentScope);
+ });
+ if (!Result)
return false;
}
return true;
diff --git a/clang/lib/CodeGen/VarBypassDetector.h b/clang/lib/CodeGen/VarBypassDetector.h
index 164e88c0b2f1b2..cc4d387aeaa5ba 100644
--- a/clang/lib/CodeGen/VarBypassDetector.h
+++ b/clang/lib/CodeGen/VarBypassDetector.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CLANG_LIB_CODEGEN_VARBYPASSDETECTOR_H
#define LLVM_CLANG_LIB_CODEGEN_VARBYPASSDETECTOR_H
+#include "CodeGenModule.h"
#include "clang/AST/Decl.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
@@ -50,7 +51,7 @@ class VarBypassDetector {
bool AlwaysBypassed = false;
public:
- void Init(const Stmt *Body);
+ void Init(CodeGenModule &CGM, const Stmt *Body);
/// Returns true if the variable declaration was by bypassed by any goto or
/// switch statement.
@@ -59,8 +60,10 @@ class VarBypassDetector {
}
private:
- bool BuildScopeInformation(const Decl *D, unsigned &ParentScope);
- bool BuildScopeInformation(const Stmt *S, unsigned &origParentScope);
+ bool BuildScopeInformation(CodeGenModule &CGM, const Decl *D,
+ unsigned &ParentScope);
+ bool BuildScopeInformation(CodeGenModule &CGM, const Stmt *S,
+ unsigned &origParentScope);
void Detect();
void Detect(unsigned From, unsigned To);
};
diff --git a/clang/test/CodeGen/deeply-nested-expressions.cpp b/clang/test/CodeGen/deeply-nested-expressions.cpp
new file mode 100644
index 00000000000000..98980be7506481
--- /dev/null
+++ b/clang/test/CodeGen/deeply-nested-expressions.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -Wstack-exhausted
+// RUN: %clang_cc1 %s -emit-llvm -o - -Wstack-exhausted -O2
+
+class AClass {
+public:
+ AClass() {}
+ AClass &f() { return *this; }
+};
+
+#define CALLS1 f
+#define CALLS2 CALLS1().CALLS1
+#define CALLS4 CALLS2().CALLS2
+#define CALLS8 CALLS4().CALLS4
+#define CALLS16 CALLS8().CALLS8
+#define CALLS32 CALLS16().CALLS16
+#define CALLS64 CALLS32().CALLS32
+#define CALLS128 CALLS64().CALLS64
+#define CALLS256 CALLS128().CALLS128
+#define CALLS512 CALLS256().CALLS256
+#define CALLS1024 CALLS512().CALLS512
+#define CALLS2048 CALLS1024().CALLS1024
+#define CALLS4096 CALLS2048().CALLS2048
+#define CALLS8192 CALLS4096().CALLS4096
+#define CALLS16384 CALLS8192().CALLS8192
+#define CALLS32768 CALLS16384().CALLS16384
+
+void test_bar() {
+ AClass a;
+ a.CALLS32768();
+}
|
ilya-biryukov
approved these changes
Jan 30, 2025
There was a problem hiding this comment.
This LG, but I'd also ask for someone who owns this CodeGen to take a look before submitting. @cor3ntin can definitely vouch for that.
The thing that gives me a pause is that we might be missing many more places that are recursive during CodeGen and it'd be nice to align that it's a problem worth solving and that putting runWithSufficientStackSpace everywhere is something people are okay with.
Clang crashing is bad, but one could argue that we are making the code more convoluted to give good error messages and prevent the crashes in some cases.
It would be great to ensure people are aware of these readability costs and are on board (even if they are small).
| @@ -0,0 +1,30 @@ | |||
| // RUN: %clang_cc1 %s -emit-llvm -o - -Wstack-exhausted | |||
There was a problem hiding this comment.
Flagging to remove this before merging since you say it's too slow.
(You've mentioned that in the description too)
PS we could probably add some build flag to optionally enable slow tests like this and set up a buildbot that runs them for all submitted changes.
It's not great, but better than nothing.
The suggestion is unrelated to this PR, just thinking out loud here.
There was a problem hiding this comment.
What if rather than removing the test we just add a REQUIRES: slow_tests line, and have a CMake option to enable the "slow_tests" feature?
That would be a really small addition to the PR and would be really nice to not lose.
|
Please let me know if you have any comments. |
8 tasks
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/199/builds/2113 Here is the relevant piece of the build log for the reference |
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.
This happens when using
-O2.Similarly to #111701 (test), not adding a test that reproduces since this test is slow and likely to be hard to maintained as discussed here and in previous discussion.
Test that was reverted here: d6b5576