[libc][test] Condition out tests that can't work on bare metal - #215830
[libc][test] Condition out tests that can't work on bare metal#215830statham-arm wants to merge 7 commits into
Conversation
In bare-metal builds of libc, the `EXPECT_DEATH` macro may not be defined. Also, `signal-macros.h` may not define the values needed for the rest of `<signal.h>` to work. So tests that rely on either of those things will fail to compile. I've conditioned out the `EXPECT_DEATH` tests completely if `EXPECT_DEATH` isn't defined. There's inherently no reliable way to define it: you can't rely on finding out about segmentation faults by a signal, because accessing memory outside valid C objects might silently succeed (valid unused memory), or generate a CPU fault that no kernel traps for you, or overwrite something important outside your program. The check for signals in `FPExceptMatcher.cpp` can be more lenient, and just condition out the signal-handling code, leaving the test of cumulative FP exception flags in place, so that the checker simply returns "no signal was caught" unconditionally.
|
@llvm/pr-subscribers-libc Author: Simon Tatham (statham-arm) ChangesIn bare-metal builds of libc, the I've conditioned out the The check for signals in Full diff: https://github.com/llvm/llvm-project/pull/215830.diff 4 Files Affected:
diff --git a/libc/test/UnitTest/FPExceptMatcher.cpp b/libc/test/UnitTest/FPExceptMatcher.cpp
index 10b6ff9f91da1..863811841124f 100644
--- a/libc/test/UnitTest/FPExceptMatcher.cpp
+++ b/libc/test/UnitTest/FPExceptMatcher.cpp
@@ -20,7 +20,15 @@
#include "hdr/types/fenv_t.h"
#include "src/__support/FPUtil/FEnvImpl.h"
#include <setjmp.h>
+
+// To make this test work on bare-metal targets without working signal.h, find
+// out if signal-macros.h did anything, before including the full signal.h. It
+// doesn't define a specific macro of the form HAVE_SIGNALS, so we just test
+// for one of the macros it _does_ define.
+#include "llvm-libc-macros/signal-macros.h"
+#ifdef __NSIGSET_WORDS
#include <signal.h>
+#endif
#if LIBC_TEST_HAS_MATCHERS()
@@ -37,27 +45,38 @@ namespace testing {
using sighandler_t = __sighandler_t *;
#endif
-static thread_local sigjmp_buf jumpBuffer;
static thread_local bool caughtExcept;
+#ifdef __NSIGSET_WORDS
+
+static thread_local sigjmp_buf jumpBuffer;
+
static void sigfpeHandler([[maybe_unused]] int sig) {
caughtExcept = true;
siglongjmp(jumpBuffer, -1);
}
+#endif // __NSIGSET_WORDS
+
FPExceptMatcher::FPExceptMatcher(FunctionCaller *func) {
+#ifdef __NSIGSET_WORDS
auto *oldSIGFPEHandler = signal(SIGFPE, &sigfpeHandler);
+#endif
caughtExcept = false;
fenv_t oldEnv;
fputil::get_env(&oldEnv);
+#ifdef __NSIGSET_WORDS
if (sigsetjmp(jumpBuffer, 1) == 0)
+#endif
func->call();
delete func;
// We restore the previous floating point environment after
// the call to the function which can potentially raise SIGFPE.
fputil::set_env(&oldEnv);
+#ifdef __NSIGSET_WORDS
signal(SIGFPE, oldSIGFPEHandler);
+#endif
exceptionRaised = caughtExcept;
}
diff --git a/libc/test/src/compiler/stack_chk_guard_test.cpp b/libc/test/src/compiler/stack_chk_guard_test.cpp
index 301031ff47bd5..978e889263405 100644
--- a/libc/test/src/compiler/stack_chk_guard_test.cpp
+++ b/libc/test/src/compiler/stack_chk_guard_test.cpp
@@ -10,6 +10,13 @@
#include "src/compiler/__stack_chk_fail.h"
#include "test/UnitTest/Test.h"
+#ifdef EXPECT_DEATH
TEST(LlvmLibcStackChkFail, Death) {
EXPECT_DEATH([] { __stack_chk_fail(); }, WITH_SIGNAL(SIGABRT));
}
+#else
+TEST(LlvmLibcStackChkFail, Dummy) {
+ // Need at least one test, because a completely empty test file
+ // counts as failure
+}
+#endif // EXPECT_DEATH
diff --git a/libc/test/src/time/localtime_r_test.cpp b/libc/test/src/time/localtime_r_test.cpp
index bc71419d68c64..e3c3b96278b89 100644
--- a/libc/test/src/time/localtime_r_test.cpp
+++ b/libc/test/src/time/localtime_r_test.cpp
@@ -27,6 +27,7 @@ TEST(LlvmLibcLocaltimeR, ValidUnixTimestamp0) {
ASSERT_EQ(0, result->tm_isdst);
}
+#ifdef EXPECT_DEATH
TEST(LlvmLibcLocaltimeR, NullPtr) {
struct tm input;
time_t timer = 0;
@@ -37,6 +38,7 @@ TEST(LlvmLibcLocaltimeR, NullPtr) {
EXPECT_DEATH([&] { LIBC_NAMESPACE::localtime_r(&timer, nullptr); },
WITH_SIGNAL(-1));
}
+#endif // EXPECT_DEATH
// TODO(zimirza): These tests does not expect the correct output of localtime as
// per specification. This is due to timezone functions removed from
diff --git a/libc/test/src/time/localtime_test.cpp b/libc/test/src/time/localtime_test.cpp
index 37974d27771dc..6e1116b98eeb1 100644
--- a/libc/test/src/time/localtime_test.cpp
+++ b/libc/test/src/time/localtime_test.cpp
@@ -25,9 +25,11 @@ TEST(LlvmLibcLocaltime, ValidUnixTimestamp0) {
ASSERT_EQ(0, result->tm_isdst);
}
+#ifdef EXPECT_DEATH
TEST(LlvmLibcLocaltime, NullPtr) {
EXPECT_DEATH([] { LIBC_NAMESPACE::localtime(nullptr); }, WITH_SIGNAL(-1));
}
+#endif // EXPECT_DEATH
// TODO(zimirza): These tests does not expect the correct output of localtime as
// per specification. This is due to timezone functions removed from
|
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
| #include "src/compiler/__stack_chk_fail.h" | ||
| #include "test/UnitTest/Test.h" | ||
|
|
||
| #ifdef EXPECT_DEATH |
There was a problem hiding this comment.
What if you changed the macro definition to be empty in case the target is baremetal (LIBC_TARGET_OS_IS_BAREMETAL)? Instead of adding these ifdefs.
There was a problem hiding this comment.
We already set them to no-op when LIBC_TEST_SKIP_DEATH_TESTS is set:
https://github.com/llvm/llvm-project/blob/main/libc/test/UnitTest/LibcTest.h#L510
|
Hmm, this doesn't seem to work, because in the CI build, the only |
|
for the death tests, can we set |
|
Ah, thanks. Yes, that looks like a better plan, and avoids the special case of the otherwise empty file. But I'm still stuck on figuring out how to avoid using |
Apparently there are platforms which don't define LIBC_TEST_SUBPROCESS_TESTS, which means that EXPECT_DEATH wouldn't have been defined in that configuration at all. I assume those platforms avoided a compile failure by not _calling_ EXPECT_DEATH. In that case my #error was overzealous: it's only an error not to define EXPECT_DEATH if you're also running a test that calls it.
|
Sorry for all the spam while I fixed the CI failures! On the plus side, I'm impressed by the thoroughness of libc CI. 🙂 I think this is now ready for a human to look at again. The conditioning-out of |
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
I'm sorry about the breakage. I'm still fairly new here, so take my comments with a grain of salt.
| add_custom_target(libc-integration-tests-build) | ||
| add_custom_target(libc_include_tests-build) | ||
|
|
||
| # Allow compiled tests to check for the existence of particular headers in this |
There was a problem hiding this comment.
I think it'd be better to have a dedicated macro for what you're trying to achieve (if we need one), instead of full suite of definitions like this. This does not even precisely describe the property you want to capture. We allow targets to customize the set of entry points in a header, so you could hypothetically have a target which defines signal.h, but still does not have all the required libc functions.
| # Set LIBC_TEST_SKIP_DEATH_TESTS to skip running tests that use EXPECT_DEATH | ||
| # and ASSERT_DEATH. On platforms where they work, they can be slow; on | ||
| # bare-metal platforms it might not be possible to implement them at all. | ||
| if(LIBC_TEST_SKIP_DEATH_TESTS) |
There was a problem hiding this comment.
I think it'd be better to keep this inside the LIBC_TEST_SUBPROCESS_TESTS branch since death tests (at least how they are implemented right now) are a special kind of a subprocess test.
What is the story with overall subprocess (e.g. EXPECT_EXIT) tests for bare metal targets. If you don't support those either, maybe we should be the behavior for subprocess tests in general?
There was a problem hiding this comment.
You mean, leave the existing ifdefs as they originally were, but whenever LIBC_TEST_SUBPROCESS_TESTS isn't defined, we have a second #else branch where we define EXPECT_DEATH to be empty? OK.
We aren't seeing any test-program build failures relating to EXPECT_EXIT (in that with this patch and Mark's linked one we see no failures at all). I think that's just because the EXPECT_EXIT tests are all specific to functions that deal with complicated exiting behaviour, like atexit, and the bare-metal libc configuration doesn't include those functions, hence the tests aren't built or run either.
Whereas EXPECT_DEATH is more widespread, because it appears in tests of functions that have nothing per se to do with exiting, such as testing that if you pass a null pointer to localtime then something appropriately horrible happens.
Sorry for the FreeBSD pipeline being "annoying" :D. I added that for the team to track "does libc work on other POSIX platforms". Do let me know if any freebsd stuff is being naughty again -- I will try to improve them later. |
| # Check if tests can safely include signal.h. (FPExceptMatcher.cpp can | ||
| # use it, but can also do without it.) | ||
| if(NOT LLVM_LIBC_FULL_BUILD) | ||
| # In an overlay build of libc, assume signal.h is available: the host libc | ||
| # will provide it even if we don't. | ||
| add_compile_definitions(HAVE_SIGNAL_H) | ||
| else() | ||
| # In a full build, signal.h only exists if we provide it ourselves. | ||
| # In a bare-metal full build we might not, so check which headers | ||
| # we're shipping. | ||
| list(FIND TARGET_PUBLIC_HEADERS libc.include.signal SIGNAL_H_idx) | ||
| if(SIGNAL_H_idx GREATER -1) | ||
| add_compile_definitions(HAVE_SIGNAL_H) | ||
| endif() | ||
| endif() | ||
|
|
There was a problem hiding this comment.
It would be better to check the presence of the actual entry points rather than of the header (this is what I meant with the first comment -- sorry I wasn't clear). Also, would it be possible to put this next to the clock checking code in
llvm-project/libc/test/UnitTest/CMakeLists.txt
Lines 23 to 25 in a26fae5
.. and call it something like TARGET_SUPPORTS_SIGNAL_CATCHING?
5 participants
In bare-metal builds of libc, the
EXPECT_DEATHmacro may not be defined. Also,signal-macros.hmay not define the values needed for the rest of<signal.h>to work. So tests that rely on either of those things will fail to compile.I've conditioned out the
EXPECT_DEATHtests completely ifEXPECT_DEATHisn't defined. There's inherently no reliable way to define it: you can't rely on finding out about segmentation faults by a signal, because accessing memory outside valid C objects might silently succeed (valid unused memory), or generate a CPU fault that no kernel traps for you, or overwrite something important outside your program.The check for signals in
FPExceptMatcher.cppcan be more lenient, and just condition out the signal-handling code, leaving the test of cumulative FP exception flags in place, so that the checker simply returns "no signal was caught" unconditionally.