[libc] Properly clear out errno in tests that check its value. #131650
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make sure to set libc_errno to zero before calling the functions that may modify it, and subsequent errno verification. The tests modified below became more brittle after the change in f9146cc which enabled system errno for unit tests (system errno is more likely to be set to a non-null value before the tests even begin to run).
|
@llvm/pr-subscribers-libc Author: Alexey Samsonov (vonosmas) ChangesMake sure to set libc_errno to zero before calling the functions that may modify it, and subsequent errno verification. The tests modified below became more brittle after the change in Full diff: https://github.com/llvm/llvm-project/pull/131650.diff 15 Files Affected:
diff --git a/libc/test/src/math/smoke/FModTest.h b/libc/test/src/math/smoke/FModTest.h
index 8fbcc2a276542..e63bbced545b3 100644
--- a/libc/test/src/math/smoke/FModTest.h
+++ b/libc/test/src/math/smoke/FModTest.h
@@ -19,10 +19,10 @@
#define TEST_SPECIAL(x, y, expected, dom_err, expected_exception) \
LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT); \
+ LIBC_NAMESPACE::libc_errno = 0; \
EXPECT_FP_EQ(expected, f(x, y)); \
EXPECT_MATH_ERRNO((dom_err) ? EDOM : 0); \
- EXPECT_FP_EXCEPTION(expected_exception); \
- LIBC_NAMESPACE::fputil::clear_except(FE_ALL_EXCEPT)
+ EXPECT_FP_EXCEPTION(expected_exception)
#define TEST_REGULAR(x, y, expected) TEST_SPECIAL(x, y, expected, false, 0)
diff --git a/libc/test/src/sys/epoll/linux/epoll_create1_test.cpp b/libc/test/src/sys/epoll/linux/epoll_create1_test.cpp
index 4059afe16b807..b3b04cd601134 100644
--- a/libc/test/src/sys/epoll/linux/epoll_create1_test.cpp
+++ b/libc/test/src/sys/epoll/linux/epoll_create1_test.cpp
@@ -15,6 +15,7 @@
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
TEST(LlvmLibcEpollCreate1Test, Basic) {
+ LIBC_NAMESPACE::libc_errno = 0;
int fd = LIBC_NAMESPACE::epoll_create1(0);
ASSERT_GT(fd, 0);
ASSERT_ERRNO_SUCCESS();
@@ -23,6 +24,7 @@ TEST(LlvmLibcEpollCreate1Test, Basic) {
}
TEST(LlvmLibcEpollCreate1Test, CloseOnExecute) {
+ LIBC_NAMESPACE::libc_errno = 0;
int fd = LIBC_NAMESPACE::epoll_create1(EPOLL_CLOEXEC);
ASSERT_GT(fd, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/epoll/linux/epoll_create_test.cpp b/libc/test/src/sys/epoll/linux/epoll_create_test.cpp
index 9c4bad10c8384..8d048d38037b2 100644
--- a/libc/test/src/sys/epoll/linux/epoll_create_test.cpp
+++ b/libc/test/src/sys/epoll/linux/epoll_create_test.cpp
@@ -15,6 +15,7 @@
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
TEST(LlvmLibcEpollCreateTest, Basic) {
+ LIBC_NAMESPACE::libc_errno = 0;
int fd = LIBC_NAMESPACE::epoll_create(1);
ASSERT_GT(fd, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/epoll/linux/epoll_ctl_test.cpp b/libc/test/src/sys/epoll/linux/epoll_ctl_test.cpp
index fa2d358c57966..067b145b95d78 100644
--- a/libc/test/src/sys/epoll/linux/epoll_ctl_test.cpp
+++ b/libc/test/src/sys/epoll/linux/epoll_ctl_test.cpp
@@ -19,6 +19,7 @@
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
TEST(LlvmLibcEpollCtlTest, Basic) {
+ LIBC_NAMESPACE::libc_errno = 0;
int epfd = LIBC_NAMESPACE::epoll_create1(0);
ASSERT_GT(epfd, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/epoll/linux/epoll_pwait2_test.cpp b/libc/test/src/sys/epoll/linux/epoll_pwait2_test.cpp
index 2f4c9854be1a0..1246a3c5a0f06 100644
--- a/libc/test/src/sys/epoll/linux/epoll_pwait2_test.cpp
+++ b/libc/test/src/sys/epoll/linux/epoll_pwait2_test.cpp
@@ -20,6 +20,7 @@
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
TEST(LlvmLibcEpollPwaitTest, Basic) {
+ LIBC_NAMESPACE::libc_errno = 0;
int epfd = LIBC_NAMESPACE::epoll_create1(0);
ASSERT_GT(epfd, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/epoll/linux/epoll_pwait_test.cpp b/libc/test/src/sys/epoll/linux/epoll_pwait_test.cpp
index 8e14aea8b9d57..abfbd4053a526 100644
--- a/libc/test/src/sys/epoll/linux/epoll_pwait_test.cpp
+++ b/libc/test/src/sys/epoll/linux/epoll_pwait_test.cpp
@@ -20,6 +20,7 @@
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
TEST(LlvmLibcEpollPwaitTest, Basic) {
+ LIBC_NAMESPACE::libc_errno = 0;
int epfd = LIBC_NAMESPACE::epoll_create1(0);
ASSERT_GT(epfd, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/epoll/linux/epoll_wait_test.cpp b/libc/test/src/sys/epoll/linux/epoll_wait_test.cpp
index f9e855a891b05..e5b7c89417798 100644
--- a/libc/test/src/sys/epoll/linux/epoll_wait_test.cpp
+++ b/libc/test/src/sys/epoll/linux/epoll_wait_test.cpp
@@ -19,6 +19,7 @@
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
TEST(LlvmLibcEpollWaitTest, Basic) {
+ LIBC_NAMESPACE::libc_errno = 0;
int epfd = LIBC_NAMESPACE::epoll_create1(0);
ASSERT_GT(epfd, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/socket/linux/send_recv_test.cpp b/libc/test/src/sys/socket/linux/send_recv_test.cpp
index a5d4880d934cc..bc2184e261a5b 100644
--- a/libc/test/src/sys/socket/linux/send_recv_test.cpp
+++ b/libc/test/src/sys/socket/linux/send_recv_test.cpp
@@ -23,17 +23,19 @@ TEST(LlvmLibcSendRecvTest, SucceedsWithSocketPair) {
int sockpair[2] = {0, 0};
+ LIBC_NAMESPACE::libc_errno = 0;
int result = LIBC_NAMESPACE::socketpair(AF_UNIX, SOCK_STREAM, 0, sockpair);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t send_result =
LIBC_NAMESPACE::send(sockpair[0], TEST_MESSAGE, MESSAGE_LEN, 0);
EXPECT_EQ(send_result, static_cast<ssize_t>(MESSAGE_LEN));
ASSERT_ERRNO_SUCCESS();
+ LIBC_NAMESPACE::libc_errno = 0;
char buffer[256];
-
ssize_t recv_result =
LIBC_NAMESPACE::recv(sockpair[1], buffer, sizeof(buffer), 0);
ASSERT_EQ(recv_result, static_cast<ssize_t>(MESSAGE_LEN));
@@ -42,10 +44,12 @@ TEST(LlvmLibcSendRecvTest, SucceedsWithSocketPair) {
ASSERT_STREQ(buffer, TEST_MESSAGE);
// close both ends of the socket
+ LIBC_NAMESPACE::libc_errno = 0;
result = LIBC_NAMESPACE::close(sockpair[0]);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
+ LIBC_NAMESPACE::libc_errno = 0;
result = LIBC_NAMESPACE::close(sockpair[1]);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
@@ -55,19 +59,17 @@ TEST(LlvmLibcSendRecvTest, SendFails) {
const char TEST_MESSAGE[] = "connection terminated";
const size_t MESSAGE_LEN = sizeof(TEST_MESSAGE);
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t send_result = LIBC_NAMESPACE::send(-1, TEST_MESSAGE, MESSAGE_LEN, 0);
EXPECT_EQ(send_result, ssize_t(-1));
ASSERT_ERRNO_FAILURE();
-
- LIBC_NAMESPACE::libc_errno = 0; // reset errno to avoid test ordering issues.
}
TEST(LlvmLibcSendRecvTest, RecvFails) {
char buffer[256];
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t recv_result = LIBC_NAMESPACE::recv(-1, buffer, sizeof(buffer), 0);
ASSERT_EQ(recv_result, ssize_t(-1));
ASSERT_ERRNO_FAILURE();
-
- LIBC_NAMESPACE::libc_errno = 0; // reset errno to avoid test ordering issues.
}
diff --git a/libc/test/src/sys/socket/linux/sendmsg_recvmsg_test.cpp b/libc/test/src/sys/socket/linux/sendmsg_recvmsg_test.cpp
index abcb0a3e6e506..0ae07e124e5e1 100644
--- a/libc/test/src/sys/socket/linux/sendmsg_recvmsg_test.cpp
+++ b/libc/test/src/sys/socket/linux/sendmsg_recvmsg_test.cpp
@@ -23,6 +23,7 @@ TEST(LlvmLibcSendMsgRecvMsgTest, SucceedsWithSocketPair) {
int sockpair[2] = {0, 0};
+ LIBC_NAMESPACE::libc_errno = 0;
int result = LIBC_NAMESPACE::socketpair(AF_UNIX, SOCK_STREAM, 0, sockpair);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
@@ -41,6 +42,7 @@ TEST(LlvmLibcSendMsgRecvMsgTest, SucceedsWithSocketPair) {
send_message.msg_controllen = 0;
send_message.msg_flags = 0;
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t send_result = LIBC_NAMESPACE::sendmsg(sockpair[0], &send_message, 0);
EXPECT_EQ(send_result, static_cast<ssize_t>(MESSAGE_LEN));
ASSERT_ERRNO_SUCCESS();
@@ -60,6 +62,7 @@ TEST(LlvmLibcSendMsgRecvMsgTest, SucceedsWithSocketPair) {
recv_message.msg_controllen = 0;
recv_message.msg_flags = 0;
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t recv_result = LIBC_NAMESPACE::recvmsg(sockpair[1], &recv_message, 0);
ASSERT_EQ(recv_result, static_cast<ssize_t>(MESSAGE_LEN));
ASSERT_ERRNO_SUCCESS();
@@ -67,10 +70,12 @@ TEST(LlvmLibcSendMsgRecvMsgTest, SucceedsWithSocketPair) {
ASSERT_STREQ(buffer, TEST_MESSAGE);
// close both ends of the socket
+ LIBC_NAMESPACE::libc_errno = 0;
result = LIBC_NAMESPACE::close(sockpair[0]);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
+ LIBC_NAMESPACE::libc_errno = 0;
result = LIBC_NAMESPACE::close(sockpair[1]);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
@@ -94,11 +99,10 @@ TEST(LlvmLibcSendMsgRecvMsgTest, SendFails) {
send_message.msg_controllen = 0;
send_message.msg_flags = 0;
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t send_result = LIBC_NAMESPACE::sendmsg(-1, &send_message, 0);
EXPECT_EQ(send_result, ssize_t(-1));
ASSERT_ERRNO_FAILURE();
-
- LIBC_NAMESPACE::libc_errno = 0; // reset errno to avoid test ordering issues.
}
TEST(LlvmLibcSendMsgRecvMsgTest, RecvFails) {
@@ -117,9 +121,8 @@ TEST(LlvmLibcSendMsgRecvMsgTest, RecvFails) {
recv_message.msg_controllen = 0;
recv_message.msg_flags = 0;
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t recv_result = LIBC_NAMESPACE::recvmsg(-1, &recv_message, 0);
ASSERT_EQ(recv_result, ssize_t(-1));
ASSERT_ERRNO_FAILURE();
-
- LIBC_NAMESPACE::libc_errno = 0; // reset errno to avoid test ordering issues.
}
diff --git a/libc/test/src/sys/socket/linux/sendto_recvfrom_test.cpp b/libc/test/src/sys/socket/linux/sendto_recvfrom_test.cpp
index e91b333deac58..cca5ceb1ec90f 100644
--- a/libc/test/src/sys/socket/linux/sendto_recvfrom_test.cpp
+++ b/libc/test/src/sys/socket/linux/sendto_recvfrom_test.cpp
@@ -23,10 +23,12 @@ TEST(LlvmLibcSendToRecvFromTest, SucceedsWithSocketPair) {
int sockpair[2] = {0, 0};
+ LIBC_NAMESPACE::libc_errno = 0;
int result = LIBC_NAMESPACE::socketpair(AF_UNIX, SOCK_STREAM, 0, sockpair);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t send_result = LIBC_NAMESPACE::sendto(sockpair[0], TEST_MESSAGE,
MESSAGE_LEN, 0, nullptr, 0);
EXPECT_EQ(send_result, static_cast<ssize_t>(MESSAGE_LEN));
@@ -34,6 +36,7 @@ TEST(LlvmLibcSendToRecvFromTest, SucceedsWithSocketPair) {
char buffer[256];
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t recv_result = LIBC_NAMESPACE::recvfrom(sockpair[1], buffer,
sizeof(buffer), 0, nullptr, 0);
ASSERT_EQ(recv_result, static_cast<ssize_t>(MESSAGE_LEN));
@@ -42,10 +45,12 @@ TEST(LlvmLibcSendToRecvFromTest, SucceedsWithSocketPair) {
ASSERT_STREQ(buffer, TEST_MESSAGE);
// close both ends of the socket
+ LIBC_NAMESPACE::libc_errno = 0;
result = LIBC_NAMESPACE::close(sockpair[0]);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
+ LIBC_NAMESPACE::libc_errno = 0;
result = LIBC_NAMESPACE::close(sockpair[1]);
ASSERT_EQ(result, 0);
ASSERT_ERRNO_SUCCESS();
@@ -55,21 +60,19 @@ TEST(LlvmLibcSendToRecvFromTest, SendToFails) {
const char TEST_MESSAGE[] = "connection terminated";
const size_t MESSAGE_LEN = sizeof(TEST_MESSAGE);
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t send_result =
LIBC_NAMESPACE::sendto(-1, TEST_MESSAGE, MESSAGE_LEN, 0, nullptr, 0);
EXPECT_EQ(send_result, ssize_t(-1));
ASSERT_ERRNO_FAILURE();
-
- LIBC_NAMESPACE::libc_errno = 0; // reset errno to avoid test ordering issues.
}
TEST(LlvmLibcSendToRecvFromTest, RecvFromFails) {
char buffer[256];
+ LIBC_NAMESPACE::libc_errno = 0;
ssize_t recv_result =
LIBC_NAMESPACE::recvfrom(-1, buffer, sizeof(buffer), 0, nullptr, 0);
ASSERT_EQ(recv_result, ssize_t(-1));
ASSERT_ERRNO_FAILURE();
-
- LIBC_NAMESPACE::libc_errno = 0; // reset errno to avoid test ordering issues.
}
diff --git a/libc/test/src/sys/socket/linux/socket_test.cpp b/libc/test/src/sys/socket/linux/socket_test.cpp
index d1197fa3ef7c6..63a8783452bb9 100644
--- a/libc/test/src/sys/socket/linux/socket_test.cpp
+++ b/libc/test/src/sys/socket/linux/socket_test.cpp
@@ -16,6 +16,7 @@
#include <sys/socket.h> // For AF_UNIX and SOCK_DGRAM
TEST(LlvmLibcSocketTest, LocalSocket) {
+ LIBC_NAMESPACE::libc_errno = 0;
int sock = LIBC_NAMESPACE::socket(AF_UNIX, SOCK_DGRAM, 0);
ASSERT_GE(sock, 0);
ASSERT_ERRNO_SUCCESS();
diff --git a/libc/test/src/sys/socket/linux/socketpair_test.cpp b/libc/test/src/sys/socket/linux/socketpair_test.cpp
index 9393ddd5c19d8..bc530696485af 100644
--- a/libc/test/src/sys/socket/linux/socketpair_test.cpp
+++ b/libc/test/src/sys/socket/linux/socketpair_test.cpp
@@ -16,6 +16,7 @@
#include <sys/socket.h> // For AF_UNIX and SOCK_DGRAM
TEST(LlvmLibcSocketPairTest, LocalSocket) {
+ LIBC_NAMESPACE::libc_errno = 0;
int sockpair[2] = {-1, -1};
int result = LIBC_NAMESPACE::socketpair(AF_UNIX, SOCK_DGRAM, 0, sockpair);
ASSERT_EQ(result, 0);
@@ -24,12 +25,14 @@ TEST(LlvmLibcSocketPairTest, LocalSocket) {
ASSERT_GE(sockpair[0], 0);
ASSERT_GE(sockpair[1], 0);
+ LIBC_NAMESPACE::libc_errno = 0;
LIBC_NAMESPACE::close(sockpair[0]);
LIBC_NAMESPACE::close(sockpair[1]);
ASSERT_ERRNO_SUCCESS();
}
TEST(LlvmLibcSocketPairTest, SocketFails) {
+ LIBC_NAMESPACE::libc_errno = 0;
int sockpair[2] = {-1, -1};
int result = LIBC_NAMESPACE::socketpair(-1, -1, -1, sockpair);
ASSERT_EQ(result, -1);
diff --git a/libc/test/src/unistd/pread_pwrite_test.cpp b/libc/test/src/unistd/pread_pwrite_test.cpp
index 397a0da1327a5..b777e91c4e90a 100644
--- a/libc/test/src/unistd/pread_pwrite_test.cpp
+++ b/libc/test/src/unistd/pread_pwrite_test.cpp
@@ -34,6 +34,7 @@ TEST(LlvmLibcUniStd, PWriteAndPReadBackTest) {
constexpr const char *FILENAME = "pread_pwrite.test";
auto TEST_FILE = libc_make_test_file_path(FILENAME);
+ LIBC_NAMESPACE::libc_errno = 0;
int fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU);
ASSERT_ERRNO_SUCCESS();
ASSERT_GT(fd, 0);
@@ -42,6 +43,7 @@ TEST(LlvmLibcUniStd, PWriteAndPReadBackTest) {
ASSERT_THAT(LIBC_NAMESPACE::fsync(fd), Succeeds(0));
ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+ LIBC_NAMESPACE::libc_errno = 0;
fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY);
ASSERT_ERRNO_SUCCESS();
ASSERT_GT(fd, 0);
@@ -50,6 +52,7 @@ TEST(LlvmLibcUniStd, PWriteAndPReadBackTest) {
ASSERT_THAT(LIBC_NAMESPACE::fsync(fd), Succeeds(0));
ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+ LIBC_NAMESPACE::libc_errno = 0;
fd = LIBC_NAMESPACE::open(TEST_FILE, O_RDONLY);
ASSERT_ERRNO_SUCCESS();
ASSERT_GT(fd, 0);
diff --git a/libc/test/src/unistd/read_write_test.cpp b/libc/test/src/unistd/read_write_test.cpp
index ba3aeff02042d..958e126b8fe9f 100644
--- a/libc/test/src/unistd/read_write_test.cpp
+++ b/libc/test/src/unistd/read_write_test.cpp
@@ -23,6 +23,7 @@ TEST(LlvmLibcUniStd, WriteAndReadBackTest) {
constexpr const char *FILENAME = "__unistd_read_write.test";
auto TEST_FILE = libc_make_test_file_path(FILENAME);
+ LIBC_NAMESPACE::libc_errno = 0;
int write_fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU);
ASSERT_ERRNO_SUCCESS();
ASSERT_GT(write_fd, 0);
@@ -33,6 +34,7 @@ TEST(LlvmLibcUniStd, WriteAndReadBackTest) {
ASSERT_THAT(LIBC_NAMESPACE::fsync(write_fd), Succeeds(0));
ASSERT_THAT(LIBC_NAMESPACE::close(write_fd), Succeeds(0));
+ LIBC_NAMESPACE::libc_errno = 0;
int read_fd = LIBC_NAMESPACE::open(TEST_FILE, O_RDONLY);
ASSERT_ERRNO_SUCCESS();
ASSERT_GT(read_fd, 0);
diff --git a/libc/test/src/unistd/unlink_test.cpp b/libc/test/src/unistd/unlink_test.cpp
index e1ffaab78147e..6e51ec145d5a8 100644
--- a/libc/test/src/unistd/unlink_test.cpp
+++ b/libc/test/src/unistd/unlink_test.cpp
@@ -19,6 +19,7 @@ TEST(LlvmLibcUnlinkTest, CreateAndUnlink) {
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
constexpr const char *FILENAME = "unlink.test";
auto TEST_FILE = libc_make_test_file_path(FILENAME);
+ LIBC_NAMESPACE::libc_errno = 0;
int write_fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU);
ASSERT_ERRNO_SUCCESS();
ASSERT_GT(write_fd, 0);
|
jhuber6
reviewed
Mar 17, 2025
No, because we need to clear out errno before the first invocation of function-under-test, and hence before the first call to |
And we can't do that in the test runner before we execute the unit test? |
|
We may potentially do this also in a TEST harness somehow (in the framework), though I'm not sure it wouldn't violate the principle of least surprise. |
|
Ha, beat me to it. I can try to see if it works -- if it does, is that smth. you'd prefer to see as a solution with less boilerplate? |
Yes, because then we don't need to worry about it in future tests and it's perfectly reasonable to assume that errno starts default initialized when we run these tests hermetically. |
|
One potential problem that I see is that llvm-libc tests actually have a framework selector in Adding @frobtech to confirm. |
| @@ -15,6 +15,7 @@ | |||
| using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher; | |||
|
|
|||
| TEST(LlvmLibcEpollCreate1Test, Basic) { | |||
| LIBC_NAMESPACE::libc_errno = 0; | |||
There was a problem hiding this comment.
imo we should be resetting errno at the end of tests instead of at the start. Ideally we should assume we start from a clean state.
There was a problem hiding this comment.
This assumption doesn't hold - we see cases when errno (and previously - fp exceptions) are set to non-zero values before the first test case is being executed -- e.g. by some code that runs pre-main(). Clearing out the state at the beginning of the tests would fix these cases and make the tests more hermetic.
There was a problem hiding this comment.
in that case this is fine, though I do wonder if we should leave the bazel build using LIBC_ERRNO_MODE_THREAD_LOCAL for test code since that way nothing other than our internal functions should touch that errno value.
There was a problem hiding this comment.
Right, so the tradeoff here is "keeping using LIBC_ERRNO_MODE_THREAD_LOCAL as it makes tests more hermeticvs. "switching toLIBC_ERRNO_MODE_SYSTEM` since that's what the overlay production build uses."
I'm kind of ambivalent, and would defer to you and @lntue here, though I would note that attempted switch uncovered some of the problems that we'd better fix anyway.
There was a problem hiding this comment.
That's a good point. In general I'd prefer to test what we're really going to use, so I'd say go to LIBC_ERRNO_MODE_SYSTEM.
|
After looking into this a bit more, I think our best bet is to create ErrnoCheckingTest subclass, similar to
@lntue - does it matches with what you've had in mind? |
That sounds good to me! Thanks! |
There was a problem hiding this comment.
adding an ErrnoCheckingTest seems good as long as it doesn't overlap with the fenv tests. I don't see any cases where it would from a quick glance but it's good to check.
If we're doing this, it might be good to also add some sort of build-time check to enforce using libc_errno only within ErrnoCheckingTest tests. Maybe a clang-tidy pass? Though we'd need to finish getting the other clang-tidy passes fixed as well.
| @@ -15,6 +15,7 @@ | |||
| using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher; | |||
|
|
|||
| TEST(LlvmLibcEpollCreate1Test, Basic) { | |||
| LIBC_NAMESPACE::libc_errno = 0; | |||
There was a problem hiding this comment.
That's a good point. In general I'd prefer to test what we're really going to use, so I'd say go to LIBC_ERRNO_MODE_SYSTEM.
|
Okay, closing this in favor of PR #131703 |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 18, 2025
…tests. (#131703) See the discussion in PR llvm/llvm-project#131650 on why we need to clear the errno at the beginning of some tests, and outlining the various solutions. Introduce ErrnoCheckingTest base class and use it for unlink_test as an example.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure to set libc_errno to zero before calling the functions that may modify it, and subsequent errno verification. The tests modified below became more brittle after the change in
f9146cc which enabled system errno for unit tests (system errno is more likely to be set to a non-null value before the tests even begin to run).