-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[clang][test] Split out staticanalyzer portion of Modules/specializations-lazy-load-parentmap-crash.cpp #151259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…-load-parentmap-crash.cpp When the static analyzer is disabled with -DCLANG_ENABLE_STATIC_ANALYZER=OFF, the newly added specializations-lazy-load-parentmap-crash.cpp test fails with: error: action RunAnalysis not compiled in -- ******************** ******************** Failed Tests (1): Clang :: Modules/specializations-lazy-load-parentmap-crash.cpp Add a 'REQUIRES: staticanalyzer' line to the test so that it does not run when the static analyzer is unavailable.
@llvm/pr-subscribers-clang-modules Author: Nathan Chancellor (nathanchance) ChangesWhen the static analyzer is disabled with error: action RunAnalysis not compiled in -- Failed Tests (1): Add a 'REQUIRES: staticanalyzer' line to the test so that it does not Full diff: https://github.com/llvm/llvm-project/pull/151259.diff 1 Files Affected:
diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
index bd07ada631355..19f9d14102903 100644
--- a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
@@ -1,3 +1,5 @@
+// REQUIRES: staticanalyzer
+//
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file --leading-lines %s %t
|
@llvm/pr-subscribers-clang Author: Nathan Chancellor (nathanchance) ChangesWhen the static analyzer is disabled with error: action RunAnalysis not compiled in -- Failed Tests (1): Add a 'REQUIRES: staticanalyzer' line to the test so that it does not Full diff: https://github.com/llvm/llvm-project/pull/151259.diff 1 Files Affected:
diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
index bd07ada631355..19f9d14102903 100644
--- a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
@@ -1,3 +1,5 @@
+// REQUIRES: staticanalyzer
+//
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file --leading-lines %s %t
|
Thanks for looking into this! Just to clarify, the test demonstrates two different ways that previously triggered a crash, and only one of them actually requires the static analyzer. I’m not sure if there’s a straightforward way to disable just that specific command when the static analyzer is off, but I wanted to mention it in case it sparks any ideas. |
Right, I was unsure if there was a way to do that other than splitting the test in some manner. My understanding is that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It will be better if you can split it. It can be achieved by copying the test and remove the corresponding RUN
lines. But I don't feel bad if you want to commit it as is since I believe the test may still be covered in a lot of cases.
I am happy to split the test if so desired, something like this? diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash-analyzer.cpp b/clang/test/Modules/specializations-lazy-load-parentmap-crash-analyzer.cpp
new file mode 100644
index 000000000000..52b86b1ec27d
--- /dev/null
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash-analyzer.cpp
@@ -0,0 +1,98 @@
+// REQUIRES: staticanalyzer
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file --leading-lines %s %t
+//
+// Prepare the BMIs.
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_a-part1.pcm %t/mod_a-part1.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_a-part2.pcm %t/mod_a-part2.cppm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_a.pcm %t/mod_a.cppm -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_b.pcm %t/mod_b.cppm -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
+
+// Trigger the construction of the parent map (which is necessary to trigger the bug this regression test is for) using ArrayBoundV2 checker:
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -analyze -analyzer-checker=security,alpha.security -analyzer-output=text %t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm -fmodule-file=mod_b=%t/mod_b.pcm
+
+//--- mod_a-part1.cppm
+module;
+namespace mod_a {
+template <int> struct Important;
+}
+
+namespace mod_a {
+Important<0>& instantiate1();
+} // namespace mod_a
+export module mod_a:part1;
+
+export namespace mod_a {
+using ::mod_a::instantiate1;
+}
+
+//--- mod_a-part2.cppm
+module;
+namespace mod_a {
+template <int> struct Important;
+}
+
+namespace mod_a {
+template <int N> Important<N>& instantiate2();
+namespace part2InternalInstantiations {
+// During the construction of the parent map, we iterate over ClassTemplateDecl::specializations() for 'Important'.
+// After GH119333, the following instantiations get loaded between the call to spec_begin() and spec_end().
+// This used to invalidate the begin iterator returned by spec_begin() by the time the end iterator is returned.
+// This is a regression test for that.
+Important<1> fn1();
+Important<2> fn2();
+Important<3> fn3();
+Important<4> fn4();
+Important<5> fn5();
+Important<6> fn6();
+Important<7> fn7();
+Important<8> fn8();
+Important<9> fn9();
+Important<10> fn10();
+Important<11> fn11();
+}
+} // namespace mod_a
+export module mod_a:part2;
+
+export namespace mod_a {
+using ::mod_a::instantiate2;
+}
+
+//--- mod_a.cppm
+export module mod_a;
+export import :part1;
+export import :part2;
+
+//--- mod_b.cppm
+export module mod_b;
+import mod_a;
+
+void a() {
+ mod_a::instantiate1();
+ mod_a::instantiate2<42>();
+}
+
+//--- test-array-bound-v2.cpp
+import mod_b;
+
+extern void someFunc(char* first, char* last);
+void triggerParentMapContextCreationThroughArrayBoundV2() {
+ // This code currently causes the ArrayBoundV2 checker to create the ParentMapContext.
+ // Once it detects an access to buf[100], the checker looks through the parents to find '&' operator.
+ // (this is needed since taking the address of past-the-end pointer is allowed by the checker)
+ char buf[100];
+ someFunc(&buf[0], &buf[100]);
+}
+
+//--- test-sanitized-build.cpp
+import mod_b;
+
+extern void some();
+void triggerParentMapContextCreationThroughSanitizedBuild(unsigned i) {
+ // This code currently causes UBSan to create the ParentMapContext.
+ // UBSan currently excludes the pattern below to avoid noise, and it relies on ParentMapContext to detect it.
+ while (i--)
+ some();
+}
diff --git a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
index 19f9d1410290..6a70b0722727 100644
--- a/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
+++ b/clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
@@ -10,10 +10,7 @@
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_a.pcm %t/mod_a.cppm -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_b.pcm %t/mod_b.cppm -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm
-// Below are two examples to trigger the construction of the parent map (which is necessary to trigger the bug this regression test is for).
-// Using ArrayBoundV2 checker:
-// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -analyze -analyzer-checker=security,alpha.security -analyzer-output=text %t/test-array-bound-v2.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm -fmodule-file=mod_b=%t/mod_b.pcm
-// Using a sanitized build:
+// Trigger the construction of the parent map (which is necessary to trigger the bug this regression test is for) using ArrayBoundV2 checker using a sanitized build:
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fsanitize=unsigned-integer-overflow -fsanitize-undefined-ignore-overflow-pattern=all -emit-llvm -o %t/ignored %t/test-sanitized-build.cpp -fmodule-file=mod_a:part2=%t/mod_a-part2.pcm -fmodule-file=mod_a=%t/mod_a.pcm -fmodule-file=mod_a:part1=%t/mod_a-part1.pcm -fmodule-file=mod_b=%t/mod_b.pcm
//--- mod_a-part1.cppm |
Yeah |
…ns-lazy-load-parentmap-crash.cpp Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Thanks, I will merge this when CI allows it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for fixing this.
I left a few minor comments. Feel free to ignore if they are not relevant.
clang/test/Modules/specializations-lazy-load-parentmap-crash.cpp
Outdated
Show resolved
Hide resolved
// RUN: split-file --leading-lines %s %t | ||
// | ||
// Prepare the BMIs. | ||
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -emit-module-interface -o %t/mod_a-part1.pcm %t/mod_a-part1.cppm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nitpick: I think that the triple argument can be removed from the commands in the ArrayBoundV2 test now that they are separate (they were only needed for the sanitized build test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, done in 5e29800.
clang/test/Modules/specializations-lazy-load-parentmap-crash-analyzer.cpp
Outdated
Show resolved
Hide resolved
…ns-lazy-load-parentmap-crash.cpp
…ns-lazy-load-parentmap-crash.cpp Signed-off-by: Nathan Chancellor <nathan@kernel.org>
…ns-lazy-load-parentmap-crash.cpp Signed-off-by: Nathan Chancellor <nathan@kernel.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for addressing all the comments @nathanchance. I added one question to make sure I understand the problem correctly.
clang/test/Modules/specializations-lazy-load-parentmap-crash-analyzer.cpp
Show resolved
Hide resolved
…ns-lazy-load-parentmap-crash.cpp Signed-off-by: Nathan Chancellor <nathan@kernel.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/21096 Here is the relevant piece of the build log for the reference
|
When the static analyzer is disabled with
-DCLANG_ENABLE_STATIC_ANALYZER=OFF, the newly added
specializations-lazy-load-parentmap-crash.cpp test fails with:
error: action RunAnalysis not compiled in
--
Failed Tests (1):
Clang :: Modules/specializations-lazy-load-parentmap-crash.cpp
Split out the part of the test that requires the static analyzer so that
it does not run when the static analyzer is unavailable.