Skip to content
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

[NFC][clang][test][asan] Make instantiation-depth-default.cpp a valid test case under asan and ubsan configs #75254

Conversation

wdunicornpro
Copy link
Contributor

Clang test instantiation-depth-default.cpp fails on Windows when built with ubsan due to extra warnings printed by the compiler:

File instantiation-depth-default.cpp Line 11: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely

The test case was disabled for asan in 571a647 because of the extra stack usage. Since ubsan also increases stack usage, seems like the two configs should be treated uniformly.

On the other hand, we might be able to re-enable this test case for asan. During some preliminary testing on Windows, Linux, and macOS with the host compiler being as old as clang-10, the test case exited successfully if the stack-exhausted warnings are suppressed, though I haven't done exhaustive testing across platforms and clang versions. Any insights into whether this change will introduce any risks to existing buildbots is appreciated.

Enabling this test case for asan helps to improve our test coverage, but if it causes problems on any buildbot, marking it as unsupported for ubsan is also a viable solution.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Dec 12, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 12, 2023

@llvm/pr-subscribers-clang

Author: Duo Wang (wdunicornpro)

Changes

Clang test instantiation-depth-default.cpp fails on Windows when built with ubsan due to extra warnings printed by the compiler:

File instantiation-depth-default.cpp Line 11: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely

The test case was disabled for asan in 571a647 because of the extra stack usage. Since ubsan also increases stack usage, seems like the two configs should be treated uniformly.

On the other hand, we might be able to re-enable this test case for asan. During some preliminary testing on Windows, Linux, and macOS with the host compiler being as old as clang-10, the test case exited successfully if the stack-exhausted warnings are suppressed, though I haven't done exhaustive testing across platforms and clang versions. Any insights into whether this change will introduce any risks to existing buildbots is appreciated.

Enabling this test case for asan helps to improve our test coverage, but if it causes problems on any buildbot, marking it as unsupported for ubsan is also a viable solution.


Full diff: https://github.com/llvm/llvm-project/pull/75254.diff

1 Files Affected:

  • (modified) clang/test/SemaTemplate/instantiation-depth-default.cpp (+5-11)
diff --git a/clang/test/SemaTemplate/instantiation-depth-default.cpp b/clang/test/SemaTemplate/instantiation-depth-default.cpp
index f5835b86b3a385..362cf2b34aef51 100644
--- a/clang/test/SemaTemplate/instantiation-depth-default.cpp
+++ b/clang/test/SemaTemplate/instantiation-depth-default.cpp
@@ -1,18 +1,12 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -ftemplate-backtrace-limit=2 %s
-//
-// FIXME: Disable this test when Clang was built with ASan, because ASan
-// increases our per-frame stack usage enough that this test no longer fits
-// within our normal stack space allocation.
-// UNSUPPORTED: asan
-//
+// RUN: %clang_cc1 -fsyntax-only -verify -ftemplate-backtrace-limit=2 -Wno-stack-exhausted %s
 // The default stack size on NetBSD is too small for this test.
 // UNSUPPORTED: system-netbsd
 
 template<int N, typename T> struct X : X<N+1, T*> {};
-// expected-error-re@11 {{recursive template instantiation exceeded maximum depth of 1024{{$}}}}
-// expected-note@11 {{instantiation of template class}}
-// expected-note@11 {{skipping 1023 contexts in backtrace}}
-// expected-note@11 {{use -ftemplate-depth=N to increase recursive template instantiation depth}}
+// expected-error-re@5 {{recursive template instantiation exceeded maximum depth of 1024{{$}}}}
+// expected-note@5 {{instantiation of template class}}
+// expected-note@5 {{skipping 1023 contexts in backtrace}}
+// expected-note@5 {{use -ftemplate-depth=N to increase recursive template instantiation depth}}
 
 X<0, int> x; // expected-note {{in instantiation of}}
 

@wdunicornpro wdunicornpro changed the title [NFC][clang][test] Make instantiation-depth-default.cpp a valid test case under asan and ubsan configs [NFC][clang][test][asan] Make instantiation-depth-default.cpp a valid test case under asan and ubsan configs Dec 14, 2023
@wdunicornpro
Copy link
Contributor Author

ping: this helps to improve our test coverage under asan and ubsan configs.

@wdunicornpro
Copy link
Contributor Author

@goussepi Do you have any thoughts on this?

// within our normal stack space allocation.
// UNSUPPORTED: asan
//
// RUN: %clang_cc1 -fsyntax-only -verify -ftemplate-backtrace-limit=2 -Wno-stack-exhausted %s
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to disable the stack-exhausted warning only if asan or ubsan? I am thinking we still might want to be able to detect stack usage regressions. It also helps documenting.
Not tested but maybe something like // RUN: %clang_cc1 -fsyntax-only -verify -ftemplate-backtrace-limit=2 %if {{asan|ubsan}} %{ -Wno-stack-exhausted %} %s
If not possible then LGTM, let me know if you need someone to commit for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. Just pushed an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the update looks good to you, could you help me commit it once the CI passes? Thanks!

Copy link
Collaborator

@goussepi goussepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@goussepi goussepi merged commit c458f92 into llvm:main Jan 5, 2024
4 checks passed
hctim added a commit that referenced this pull request Apr 2, 2024
…p` a valid test case under `asan` and `ubsan` configs (#75254)"

Disables the recursive template expansion test under ASan again. This
patch re-enabled this test with sanitizers, but it's started spuriously
failing with a stack overflow again on AArch64+ASan:
https://lab.llvm.org/buildbot/#/builders/239/builds/6363

This reverts commit c458f92.
@hctim
Copy link
Collaborator

hctim commented Apr 2, 2024

Hey folks, unfortunately this started supriously failing again on AArch64+ASan. So I've disabled it again under sanitizers (by reverting this patch): https://lab.llvm.org/buildbot/#/builders/239/builds/6363

@wdunicornpro wdunicornpro deleted the clang/test/Wno_stack_exhausted_in_instantiation_depth_default branch May 2, 2024 21:56
goussepi pushed a commit that referenced this pull request May 7, 2024
…on Windows (#91021)

Clang test `instantiation-depth-default.cpp` fails on Windows when built
with `ubsan` due to extra warnings printed by the compiler:
```console
File instantiation-depth-default.cpp Line 11: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely
```

Originally in #75254 this test
was enabled for `asan` as well but later started to cause failures in
Linux ASAN buildbots. I have excluded `asan` from this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants