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

[fuchsia][libc] Include missing macro definitions #79639

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

Caslyn
Copy link
Contributor

@Caslyn Caslyn commented Jan 26, 2024

PR 79573 introduced ASSERT_ERRNO_* macros for use in libc tests. Introduce these macro definitions to FuchsiaTest.h for string tests to compile on Fuchsia.

PR 79573 introduced `ASSERT_ERRNO_*` macros for use in libc tests.
Introduce these macro definitions to FuchsiaTest.h for string tests to
compile on Fuchsia.
@Caslyn Caslyn requested a review from frobtech January 26, 2024 19:19
@Caslyn Caslyn self-assigned this Jan 26, 2024
@llvmbot llvmbot added the libc label Jan 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 26, 2024

@llvm/pr-subscribers-libc

Author: Caslyn Tonelli (Caslyn)

Changes

PR 79573 introduced ASSERT_ERRNO_* macros for use in libc tests. Introduce these macro definitions to FuchsiaTest.h for string tests to compile on Fuchsia.


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

1 Files Affected:

  • (modified) libc/test/UnitTest/FuchsiaTest.h (+5)
diff --git a/libc/test/UnitTest/FuchsiaTest.h b/libc/test/UnitTest/FuchsiaTest.h
index 07a6e9b3f6bde5a..1b7537965813989 100644
--- a/libc/test/UnitTest/FuchsiaTest.h
+++ b/libc/test/UnitTest/FuchsiaTest.h
@@ -13,6 +13,11 @@
 
 #define WITH_SIGNAL(X) #X
 
+// These macros are used in string unittests.
+#define ASSERT_ERRNO_EQ(VAL) ASSERT_EQ(VAL, static_cast<int>(libc_errno))
+#define ASSERT_ERRNO_SUCCESS() ASSERT_EQ(0, static_cast<int>(libc_errno))
+#define ASSERT_ERRNO_FAILURE() ASSERT_NE(0, static_cast<int>(libc_errno))
+
 #ifndef EXPECT_DEATH
 // Since zxtest has ASSERT_DEATH but not EXPECT_DEATH, wrap calling it
 // in a lambda returning void to swallow any early returns so that this

@Caslyn Caslyn merged commit 6f97e6b into llvm:main Jan 26, 2024
5 checks passed
@Caslyn Caslyn deleted the add-macros-to-fuchsia-test branch January 26, 2024 21:21
@gchatelet
Copy link
Contributor

I missed updating these ones. Thx for fixing it !
None of our tests failed because the tests are within Fuchsia.

Did the Fuchsia buildbot fail on this commit?
If so can we add the libc tag to the build bot so we can see it as part of https://lab.llvm.org/buildbot/#/grid?tag=libc?

Tangentially the Fuchsia build bot takes roughly 25mn to run (according to this page) is there a way to get faster feedback? If it's faster we may be able to include it as part of our presubmit checks. @nickdesaulniers do you think it's desirable?

@nickdesaulniers
Copy link
Member

Yes, I'd like to transition more build bots to the enable runtimes build mode. @petrhosek I suspect we should be able to cross compile from a linux host for a fuchsia target? Even if it's just builds without running tests. Then we can work on playing with qemu to get the cross compiled test suites running. We could even have builds only for presubmit, then builds+tests for post submit.

@Caslyn
Copy link
Contributor Author

Caslyn commented Jan 29, 2024

Hi @gchatelet - don't worry, these kind of Fuchsia-specific fixes to libc tests are considered part of Fuchsia maintenance.

We have a particular set of rollers that integrate llvm-libc changes into Fuchsia. The way that llvm-libc is built and used in Fuchsia requires the entire Fuchsia checkout, which is big enough (~80GB) to be prohibitive to build and run on an upstream buildbot to catch these kind of errors. Also, a portion of the time the errors that we see from our libc integration roller requires a fix in Fuchsia itself and is not the fault of the llvm committer.

Our team discussed the possibility of building libc unittests against the Fuchsia sdk, but that would not have caught this error.

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.

None yet

5 participants