Skip to content

[libc] Remove LlvmLibcStackChkFail.Smash test #125919

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

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

frobtech
Copy link
Contributor

@frobtech frobtech commented Feb 5, 2025

This test was problematic, and also unnecessary. It's not really
a test of the libc functionality or ABI. That's already covered
by the LlvmLibcStackChkFail.Death test. The Smash test was in
fact just testing that the compiler produces the call in the
expected situation. That's a compiler test, not a libc test.

It's not really feasible to make a test like this both reliable
and safe. Since it's not something libc needs to test, it's not
worth trying.

This test was problematic, and also unnecessary.  It's not really
a test of the libc functionality or ABI.  That's already covered
by the LlvmLibcStackChkFail.Death test.  The Smash test was in
fact just testing that the compiler produces the call in the
expected situation.  That's a compiler test, not a libc test.

It's not really feasible to make a test like this both reliable
and safe.  Since it's not something libc needs to test, it's not
worth trying.
@frobtech frobtech marked this pull request as ready for review February 5, 2025 20:21
@llvmbot llvmbot added the libc label Feb 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

@llvm/pr-subscribers-libc

Author: Roland McGrath (frobtech)

Changes

This test was problematic, and also unnecessary. It's not really
a test of the libc functionality or ABI. That's already covered
by the LlvmLibcStackChkFail.Death test. The Smash test was in
fact just testing that the compiler produces the call in the
expected situation. That's a compiler test, not a libc test.

It's not really feasible to make a test like this both reliable
and safe. Since it's not something libc needs to test, it's not
worth trying.


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

1 Files Affected:

  • (modified) libc/test/src/compiler/stack_chk_guard_test.cpp (-15)
diff --git a/libc/test/src/compiler/stack_chk_guard_test.cpp b/libc/test/src/compiler/stack_chk_guard_test.cpp
index 4ec8398c9fc95dc..301031ff47bd5e4 100644
--- a/libc/test/src/compiler/stack_chk_guard_test.cpp
+++ b/libc/test/src/compiler/stack_chk_guard_test.cpp
@@ -7,24 +7,9 @@
 //===----------------------------------------------------------------------===//
 
 #include "hdr/signal_macros.h"
-#include "src/__support/macros/sanitizer.h"
 #include "src/compiler/__stack_chk_fail.h"
-#include "src/string/memset.h"
 #include "test/UnitTest/Test.h"
 
 TEST(LlvmLibcStackChkFail, Death) {
   EXPECT_DEATH([] { __stack_chk_fail(); }, WITH_SIGNAL(SIGABRT));
 }
-
-// Disable the test when asan is enabled so that it doesn't immediately fail
-// after the memset, but before the stack canary is re-checked.
-#ifndef LIBC_HAS_ADDRESS_SANITIZER
-TEST(LlvmLibcStackChkFail, Smash) {
-  EXPECT_DEATH(
-      [] {
-        int arr[20];
-        LIBC_NAMESPACE::memset(arr, 0xAA, 2001);
-      },
-      WITH_SIGNAL(SIGABRT));
-}
-#endif // LIBC_HAS_ADDRESS_SANITIZER

@frobtech frobtech merged commit 4eab219 into llvm:main Feb 5, 2025
14 of 17 checks passed
@frobtech frobtech deleted the p/libc-LlvmLibcStackChkFail branch February 5, 2025 20:34
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
This test was problematic, and also unnecessary.  It's not really
a test of the libc functionality or ABI.  That's already covered
by the LlvmLibcStackChkFail.Death test.  The Smash test was in
fact just testing that the compiler produces the call in the
expected situation.  That's a compiler test, not a libc test.

It's not really feasible to make a test like this both reliable
and safe.  Since it's not something libc needs to test, it's not
worth trying.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants