[Serialization] Check for stack exhaustion when reading declarations #79875
Conversation
ilya-biryukov
added
the
clang:frontend
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Ilya Biryukov (ilya-biryukov) ChangesParticular example that lead to this is a very long chain of To avoid that, check for stack exhaustion when deserializing the declaration. At that point, we can point to source location of a particular declaration that is being deserialized. Full diff: https://github.com/llvm/llvm-project/pull/79875.diff 2 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 547eb77930b4eec..e2507d7c14a15d0 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
// calls to Decl::getASTContext() by Decl's methods will find the
// TranslationUnitDecl without crashing.
D->setDeclContext(Context.getTranslationUnitDecl());
- Reader.Visit(D);
+
+ // Reading some declarations can result in deep recursion.
+ SemaObj->runWithSufficientStackSpace(DeclLoc, [&] { Reader.Visit(D); });
// If this declaration is also a declaration context, get the
// offsets for its tables of lexical and visible declarations.
diff --git a/clang/test/Modules/lots-of-using-shadow-decls.cpp b/clang/test/Modules/lots-of-using-shadow-decls.cpp
new file mode 100644
index 000000000000000..c3048352842e3f9
--- /dev/null
+++ b/clang/test/Modules/lots-of-using-shadow-decls.cpp
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/usings.cppm -o %t/usings.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=usings=%t/usings.pcm %t/use.cpp -verify -fsyntax-only -Wno-stack-exhausted
+
+// expected-no-diagnostics
+
+//--- usings.cppm
+export module usings;
+
+#define TYPES1(NAME) DECLARE(NAME##a) DECLARE(NAME##b) DECLARE(NAME##c) \
+ DECLARE(NAME##d) DECLARE(NAME##e) DECLARE(NAME##f) DECLARE(NAME##g) \
+ DECLARE(NAME##h) DECLARE(NAME##i) DECLARE(NAME##j)
+#define TYPES2(NAME) TYPES1(NAME##a) TYPES1(NAME##b) TYPES1(NAME##c) \
+ TYPES1(NAME##d) TYPES1(NAME##e) TYPES1(NAME##f) TYPES1(NAME##g) \
+ TYPES1(NAME##h) TYPES1(NAME##i) TYPES1(NAME##j)
+#define TYPES3(NAME) TYPES2(NAME##a) TYPES2(NAME##b) TYPES2(NAME##c) \
+ TYPES2(NAME##d) TYPES2(NAME##e) TYPES2(NAME##f) TYPES2(NAME##g) \
+ TYPES2(NAME##h) TYPES2(NAME##i) TYPES2(NAME##j)
+#define TYPES4(NAME) TYPES3(NAME##a) TYPES3(NAME##b) TYPES3(NAME##c) \
+ TYPES3(NAME##d) TYPES3(NAME##e) TYPES3(NAME##f) TYPES3(NAME##g)
+
+#define DECLARE(NAME) struct NAME {};
+TYPES4(Type)
+
+export struct Base {
+#undef DECLARE
+#define DECLARE(NAME) void func(NAME*);
+TYPES4(Type)
+};
+
+export struct Derived : Base {
+ using Base::func;
+};
+
+//--- use.cpp
+import usings;
+void test() {
+ Derived().func(nullptr); // expected-error{{ambiguous}}
+ // expected-note@* + {{candidate function}}
+}
|
There was a problem hiding this comment.
I think the alternative we could try to chase is turning the chain of using-shadow-decls into an array somehow, so we can process it in a loop instead of recursion.
I am not sure if there is an an approach that does not involve structural changes to the AST (i.e. rewriting the ASTReaderDecl somehow, but keeping the AST the same). Maybe someone more experienced with the serialization could suggest something useful here, that would probably be ideal.
|
Could you show the stack (omitting/annotating the repeated part) that leads to failure? and/or the AST shape that leads to failure? |
| Reader.Visit(D); | ||
|
|
||
| // Reading some declarations can result in deep recursion. | ||
| SemaObj->runWithSufficientStackSpace(DeclLoc, [&] { Reader.Visit(D); }); |
There was a problem hiding this comment.
Sema may not meaningful in ASTReader (e.g., we're compiling a pcm file).
| SemaObj->runWithSufficientStackSpace(DeclLoc, [&] { Reader.Visit(D); }); | |
| clang::runWithSufficientStackSpace(...) |
Also in this way, we can get better diagnotsics.
There was a problem hiding this comment.
Could you elaborate in which sense it is non-meaningful for the purpose of calling this method?
I believe SemaObj is never null here (because it's used during deserialization of declarations). The only logic we have in runWithSufficientStackSpace, is to warn about stack exhaustion only once and actually show this warning.
I would still be using Wstack-exhausted here, not sure there is much useful context to add here (maybe a note about deserializing declarations, but this case is so rare I think we can live without it).
There was a problem hiding this comment.
As I said, for example, if we're compiling a pcm file to an object file. It makes sense that the Sema is not needed here.
There was a problem hiding this comment.
I did not realize Sema can be null, I have updated the code as you suggested, but it now duplicates the logic we have in Sema to avoid showing the error multiple times.
Please take a look and see if the new version makes sense.
|
Another idea is to limit (or check) the threshold for |
See the test I added. All you need is ~10k overloads of a method in a class and a Feel free to just call |
Maybe... As long as we only use this in tests. I actually landed a patch internally that increased the stack size and it immediately started crashing on another test. I suspect that what happened there is that increasing the stack size causes us to have more recursive calls before we issue a warning and start a new thread. In turn, those recursive calls may lead to stack overflow before we get to the next call of runWithSufficientStackSpace. I am not saying it's a bad idea to add this for testing purposes, but given how fragile this approach is, I think we should not provide configuration knobs to users there. At least everyone sees crashes at roughly the same points right now (although variation is still present because depending on inlining decisions stacks sizes might vary even with the same code and same limits). |
Sorry for not being clear enough. What I want is to add parameters to BTW, how about the idea of using ASTReader::NumCurrentElementsDeserializing? |
Oh, sorry, I somehow missed this. I think it's a good idea and somewhat similar to what I was suggesting when mentioning a template instantiation limit we have. There are a few downsides that I see too:
So I think it's a good idea, but looks complementary to this patch. If we use I am curious about performance concerns. I feel that the amount of work in
Just to make sure I got everything right this time. So you mean testing the |
|
For the code we ran this into, we would definitely need a workaround that is slow and creates an extra thread. Unfortunately, there is no easy way to change the code generator there so it produces less methods :( |
It is a surprise to me that this is only a warning instead of a hard error. I never thought it can continue when the stack runs out. Sorry for confusing.
In my imagination, with
What's the meaning? Isn't this patch about reading?
I mean something like: First, refactor the signature of then add two setters to ASTReader then in the unittest under And we can try to parse the This is the code in my mind. I don't feel it would be pretty hard. How do you think about it? |
Yeah, I was hoping to have it in the text of the discussion here without having to do it myself since you've already got the repro locally, presumably... so we can all see/discuss it. But perhaps it's not sufficiently helpful/constructive to worry about - not sure. |
|
I think we should apply @ChuanqiXu9 suggestion and merge that without tests. At worse it's harmless, at best it solves an actual issue for users. We have precedent for not being able to test resource exhaustion fixes. |
|
Sorry for loosing track of this change. I will go through the comments and try to land this some time next week. |
I would not be opposed to landing with a test, but I think such a test would be challenging because stack exhaustion is often slow to trigger and highly machine-dependent (so I can imagine the test failing "randomly" for different configurations or under different workloads, or being a very slow test to execute). So if there's a simple, quick-to-execute, reliably-failing test we can add, then great! But if there's not, I would be fine landing without tests. |
|
@ilya-biryukov ping! |
@dwblaikie sorry, I was responding to the AST and somehow missed the stack part. I should have definitely included it from the start, my bad. Here it is: Click to expand the stack traceI have updated the code to account for @ChuanqiXu9 I would like to experiment with your idea of unit-testing this in a follow-up and land this without tests for now (people seem to be ok landing this without tests, so I hope it's not controversial). I like the idea of unit-testing this, but the amount of moving pieces needed to set this up worries me, I wonder if I can find a way to write a relatively small test for this (specifically, I'm hopeful that there will be some way to reuse existing helpers to make setting up the compiler-instance mostly boilerplate-free). I will be out until the rest of the week, but my colleague @usx95 can help land this if the PR is approved. |
|
there's a couple of tests that use |
|
(a bunch of compiler-rt tests also use ulimit, but doesn't look like any llvm core tests do... ) |
oh, I didn't realize I can use |
|
I have tried If the stack limit is very small, we crash on some other code path or even do something weird in the code path checking for stack exhaustion (it currently checks if there's less than 256K stack available, clearly not the right thing if I request a stack of 128K with ulimit in the first place). I think @ChuanqiXu9 's suggestion of making the parameters (desireableStackSize, in particular) of these checks customizable for unit testing would allow to have a more fine-grained knob and actually have a test for this. I'll try to experiment with this approach and report my findings. In the meantime, I have addressed the last comment and plan to land this without tests for now. |
Particular example that lead to this is a very long chain of `UsingShadowDecl`s that we hit in our codebase in generated code. To avoid that, check for stack exhaustion when deserializing the declaration. At that point, we can point to source location of a particular declaration that is being deserialized.
…ations Use clang::runWithSufficientStackSpace, fallback to Sema only when available.
…ations remove the test, it was there for illustrative purposes
…ations Update the comment according to the review suggestion.
Particular example that lead to this is a very long chain of
UsingShadowDecls that we hit in our codebase in generated code.To avoid that, check for stack exhaustion when deserializing the declaration. At that point, we can point to source location of a particular declaration that is being deserialized.