[libc] Use `LIBC_COPT_PUBLIC_PACKAGING` for hermetic and integration tests. #79319

lntue · 2024-01-24T16:20:09Z

Currently all unit, hermetic, and integration tests are linked against __internal__ entry points. This change switches the hermetic and integration tests to be linked against the public versions of the entry points.

llvmbot · 2024-01-24T16:20:43Z

@llvm/pr-subscribers-libc

Author: None (lntue)

Changes

Currently all unit, hermetic, and integration tests are linked against __internal__ entry points. This change switches the hermetic and integration tests to be linked against the public versions of the entry points.

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

3 Files Affected:

(modified) libc/cmake/modules/LLVMLibCTestRules.cmake (+13-7)
(modified) libc/test/integration/startup/CMakeLists.txt (+1-1)
(modified) libc/test/src/math/differential_testing/CMakeLists.txt (+1-1)

diff --git a/libc/cmake/modules/LLVMLibCTestRules.cmake b/libc/cmake/modules/LLVMLibCTestRules.cmake
index 46ffc8316fc8227..3b1293e271f0f56 100644
--- a/libc/cmake/modules/LLVMLibCTestRules.cmake
+++ b/libc/cmake/modules/LLVMLibCTestRules.cmake
@@ -5,6 +5,7 @@
 # Usage:
 #   get_object_files_for_test(<result var>
 #                             <skipped_entrypoints_var>
+#                             <internal_obj>
 #                             <target0> [<target1> ...])
 #
 #   The list of object files is collected in <result_var>.
@@ -12,7 +13,8 @@
 #   set to a true value.
 #   targetN is either an "add_entrypoint_target" target or an
 #   "add_object_library" target.
-function(get_object_files_for_test result skipped_entrypoints_list)
+#   If internal_obj is TRUE, then we collect `target.__internal__` for entry points.
+function(get_object_files_for_test result skipped_entrypoints_list internal_obj)
   set(object_files "")
   set(skipped_list "")
   set(checked_list "")
@@ -49,7 +51,7 @@ function(get_object_files_for_test result skipped_entrypoints_list)
       set(dep_skip "")
 
       get_target_property(indirect_deps ${dep} "DEPS")
-      get_object_files_for_test(dep_obj dep_skip ${indirect_deps})
+      get_object_files_for_test(dep_obj dep_skip ${internal_obj} ${indirect_deps})
 
       if(${dep_type} STREQUAL ${OBJECT_LIBRARY_TARGET_TYPE})
         get_target_property(dep_object_files ${dep} "OBJECT_FILES")
@@ -62,7 +64,11 @@ function(get_object_files_for_test result skipped_entrypoints_list)
           list(APPEND dep_skip ${dep})
           list(REMOVE_ITEM dep_obj ${dep})
         endif()
-        get_target_property(object_file_raw ${dep} "OBJECT_FILE_RAW")
+        if(${internal_obj})
+          get_target_property(object_file_raw ${dep} "OBJECT_FILE_RAW")
+        else()
+          get_target_property(object_file_raw ${dep} "OBJECT_FILE")
+        endif()
         if(object_file_raw)
           list(APPEND dep_obj ${object_file_raw})
         endif()
@@ -130,7 +136,7 @@ function(create_libc_unittest fq_target_name)
                            libc.test.UnitTest.ErrnoSetterMatcher)
   list(REMOVE_DUPLICATES fq_deps_list)
   get_object_files_for_test(
-      link_object_files skipped_entrypoints_list ${fq_deps_list})
+      link_object_files skipped_entrypoints_list TRUE ${fq_deps_list})
   if(skipped_entrypoints_list)
     # If a test is OS/target machine independent, it has to be skipped if the
     # OS/target machine combination does not provide any dependent entrypoints.
@@ -388,7 +394,7 @@ function(add_libc_fuzzer target_name)
   get_fq_target_name(${target_name} fq_target_name)
   get_fq_deps_list(fq_deps_list ${LIBC_FUZZER_DEPENDS})
   get_object_files_for_test(
-      link_object_files skipped_entrypoints_list ${fq_deps_list})
+      link_object_files skipped_entrypoints_list TRUE ${fq_deps_list})
   if(skipped_entrypoints_list)
     if(LIBC_CMAKE_VERBOSE_LOGGING)
       set(msg "Skipping fuzzer target ${fq_target_name} as it has missing deps: "
@@ -518,7 +524,7 @@ function(add_integration_test test_name)
   # TODO: Instead of gathering internal object files from entrypoints,
   # collect the object files with public names of entrypoints.
   get_object_files_for_test(
-      link_object_files skipped_entrypoints_list ${fq_deps_list})
+      link_object_files skipped_entrypoints_list FALSE ${fq_deps_list})
   if(skipped_entrypoints_list)
     if(LIBC_CMAKE_VERBOSE_LOGGING)
       set(msg "Skipping unittest ${fq_target_name} as it has missing deps: "
@@ -702,7 +708,7 @@ function(add_libc_hermetic_test test_name)
   # TODO: Instead of gathering internal object files from entrypoints,
   # collect the object files with public names of entrypoints.
   get_object_files_for_test(
-      link_object_files skipped_entrypoints_list ${fq_deps_list})
+      link_object_files skipped_entrypoints_list FALSE ${fq_deps_list})
   if(skipped_entrypoints_list)
     set(msg "Skipping unittest ${fq_target_name} as it has missing deps: "
             "${skipped_entrypoints_list}.")
diff --git a/libc/test/integration/startup/CMakeLists.txt b/libc/test/integration/startup/CMakeLists.txt
index fb5d6bc787cc261..8bdb841faf74f35 100644
--- a/libc/test/integration/startup/CMakeLists.txt
+++ b/libc/test/integration/startup/CMakeLists.txt
@@ -38,7 +38,7 @@ function(add_startup_test target_name)
   if(ADD_STARTUP_TEST_DEPENDS)
     get_fq_deps_list(fq_deps_list ${ADD_STARTUP_TEST_DEPENDS})
     add_dependencies(${fq_target_name} ${fq_deps_list})
-    get_object_files_for_test(link_object_files has_skipped_entrypoint_list ${fq_deps_list})
+    get_object_files_for_test(link_object_files has_skipped_entrypoint_list TRUE ${fq_deps_list})
     target_link_libraries(${fq_target_name} ${link_object_files})
   endif()
 
diff --git a/libc/test/src/math/differential_testing/CMakeLists.txt b/libc/test/src/math/differential_testing/CMakeLists.txt
index 72bc2f8fb16aaa0..41e1f2bcdca4945 100644
--- a/libc/test/src/math/differential_testing/CMakeLists.txt
+++ b/libc/test/src/math/differential_testing/CMakeLists.txt
@@ -27,7 +27,7 @@ function(add_diff_binary target_name)
   get_fq_target_name(${target_name} fq_target_name)
   get_fq_deps_list(fq_deps_list ${DIFF_DEPENDS})
   get_object_files_for_test(
-      link_object_files skipped_entrypoints_list ${fq_deps_list})
+      link_object_files skipped_entrypoints_list TRUE ${fq_deps_list})
   if(skipped_entrypoints_list)
     if(LIBC_CMAKE_VERBOSE_LOGGING)
       set(msg "Will not build ${fq_target_name} as it has missing deps: "

jhuber6 · 2024-01-24T16:21:39Z

I remember the GPU had to do the __internal__ stuff because we build the public release version for multiple architectures, but the tests are only built for the singular GPU architecture that the user's machine supports. Will this change that logic?

lntue · 2024-01-24T17:47:01Z

I remember the GPU had to do the __internal__ stuff because we build the public release version for multiple architectures, but the tests are only built for the singular GPU architecture that the user's machine supports. Will this change that logic?

I'll try to leave the GPU logic intact for now.

lntue · 2024-01-26T07:35:49Z

Current progress:

All tests seem to work correctly in overlay mode
Most tests seem to work correctly in full build mode - Currently check-libc failed at [3906/4117] with
projects/libc/test/src/unistd/libc.test.src.unistd.getopt_test.__hermetic__.__build__ target.

gchatelet

I know the patch is still not ready for review but I wanted to provide early feedback :)

libc/cmake/modules/LLVMLibCTestRules.cmake

libc/src/errno/CMakeLists.txt

libc/src/errno/libc_errno.cpp

libc/src/errno/libc_errno.h

libc/src/errno/libc_errno.cpp

libc/test/UnitTest/CMakeLists.txt

libc/test/UnitTest/LibcTest.cpp

gchatelet · 2024-01-26T08:45:44Z

libc/test/integration/src/pthread/pthread_create_test.cpp

@@ -47,7 +47,7 @@ static void *successThread(void *Arg) {
  pthread_t th = LIBC_NAMESPACE::pthread_self();
  auto *thread = reinterpret_cast<LIBC_NAMESPACE::Thread *>(&th);

-  ASSERT_EQ(libc_errno, 0);
+  ASSERT_EQ(static_cast<int>(libc_errno), 0);


I think this deserves an ASSERT_ERREQ macro in our gtest.
The static_cast<int> adds a lot of clutter and will be surprising to newcomers.

Given the size of this change I'd like to do it as a separate change if possible. I'll come back with such a patch and ping here.

Thanks for cleaning this up! It will make this patch much cleaner.

I'll merge with #79573 and force push this so that the libc_errno commit will be much cleaner for reviewing.

This patch provides specific test macros to deal with `errno`. This will help abstract away the differences between unit test and integration/hermetic tests in #79319. In one case we use `libc_errno` which is a struct, in the other case we deal directly with `errno`.

…sts.

…test.cpp.

…odes.

lntue · 2024-01-26T16:04:09Z

It works for both overlay and full build modes now.

jhuber6 · 2024-01-26T16:21:47Z

Breaks the GPU build on the fact that the __internal__ and public targets are different machines entirely. The public version is a fatbinary for the CPU while the internal one is a GPU image.

gchatelet

A few more comments, sorry for missing quite a few ASSERT_ERRNO_EQ macros. I think its fine to add them here.

libc/src/errno/libc_errno.cpp

libc/src/errno/libc_errno.h

gchatelet · 2024-01-26T16:21:05Z

libc/test/src/stdlib/StrtolTest.h

@@ -331,8 +331,7 @@ struct StrtoTest : public LIBC_NAMESPACE::testing::Test {
              ((is_signed_v<ReturnT> && sizeof(ReturnT) == 4)
                   ? T_MAX
                   : ReturnT(0xFFFFFFFF)));
-    ASSERT_EQ(libc_errno,
-              is_signed_v<ReturnT> && sizeof(ReturnT) == 4 ? ERANGE : 0);
+    ASSERT_ERRNO_EQ(is_signed_v<ReturnT> && sizeof(ReturnT) == 4 ? ERANGE : 0);


Thx! I missed these ones...

libc/test/src/unistd/CMakeLists.txt

lntue · 2024-01-26T17:11:57Z

Breaks the GPU build on the fact that the __internal__ and public targets are different machines entirely. The public version is a fatbinary for the CPU while the internal one is a GPU image.

Can you check again to see if the updates fix issues on GPUs?

jhuber6 · 2024-01-26T19:49:57Z

libc/test/UnitTest/CMakeLists.txt

+  # When the clock target is available, it will be used inside the test lib
+  # to indicate the performance of the tests.
+  set(clock_target "")
+  if(TARGET libc.src.time.clock)


I forget if this is broken. I think it still makes the target even if the entrypoint is disabled so you need to check the entrypoints list.

nickdesaulniers

Nice work!

nickdesaulniers · 2024-01-29T17:06:15Z

libc/include/errno.h.def

+
+#ifdef __cplusplus
+extern "C" {
+  extern thread_local int __llvmlibc_errno;


libc/src/errno/libc_errno.cpp line 18 doesn't use indentation here; I think we should remove it here, too.

nickdesaulniers · 2024-01-29T17:06:57Z

libc/src/errno/libc_errno.h

@@ -12,45 +12,34 @@
 #include "src/__support/macros/attributes.h"
 #include "src/__support/macros/properties/architectures.h"

+// TODO: Separate just the definition of errno numbers in


File a bug for this, then link to that bug in the comment?

nickdesaulniers · 2024-01-29T17:09:11Z

libc/test/src/string/CMakeLists.txt

+# TODO: For GPUs, hermetic tests still need to get linked against
+# __internal__ targets.  Update this test generator so that hermetic tests
+# work on GPUs again.


file a bug, link to it here

gchatelet · 2024-02-13T15:36:39Z

@lntue is there any blocker to landing this PR?

jhuber6 · 2024-02-13T15:52:19Z

My guess is that the blocked is me and my GPU build. However, I think very soon I'll be able to burn out all the GPU weirdness if I can get things like #81557 landed and functional. Right now the GPU build is a mess because it's actually multiple separate builds combined into one in a hacky way. My plan currently is to handle the GPU build the same way as all the other targets, but use the CMake runtimes build to make multiple invocations of libc. This will also allow using the GPU build in conjunction with a CPU build.

However, that's going to take a lot of CMake hacking to get fully working, so I wouldn't expect it immediately.

gchatelet · 2024-02-13T16:12:48Z

Ha I see. Thx for the explanation.
FYI This PR was initiated because I was able to break libc entirely without a single built bot noticing. Long story short we currently don't use the plain C functions in the integration tests (only the C++ ones). This is problematic because we don't actually check that we do ship the libc functions. Having this PR in (and the few other ones that build on it) would help me get peace of mind 🙂

gchatelet · 2024-02-29T16:25:58Z

@jhuber6 I saw quite a few PR landing over the last two weeks, do you consider the CMake GPU refactoring over?

jhuber6 · 2024-02-29T16:26:53Z

@jhuber6 I saw quite a few PR landing over the last two weeks, do you consider the CMake GPU refactoring over?

Yes, the bulk of the changes are finished. Sorry for the delay.

gchatelet · 2024-02-29T16:37:37Z

@jhuber6 I saw quite a few PR landing over the last two weeks, do you consider the CMake GPU refactoring over?

Yes, the bulk of the changes are finished. Sorry for the delay.

No worries, it's for the better :)

lntue requested review from gchatelet, frobtech, nickdesaulniers, jhuber6 and michaelrj-google January 24, 2024 16:20

llvmbot added the libc label Jan 24, 2024

lntue marked this pull request as draft January 24, 2024 17:46

gchatelet mentioned this pull request Jan 25, 2024

[reland][libc] Remove unnecessary FPBits functions and properties #79128

Merged

lntue force-pushed the test branch from c0bb6b5 to a6c9030 Compare January 26, 2024 07:29

gchatelet reviewed Jan 26, 2024

View reviewed changes

lntue added 4 commits January 26, 2024 09:49

[libc] Use LIBC_COPT_PUBLIC_PACKAGING for hermetic and integration te…

e065882

…sts.

Remove extra C functions in LibcTest.cpp, HermeticTestUtils.cpp, and …

e15ee80

…test.cpp.

Update libc_errno to work correctly for both overlay and full build m…

34a5a50

…odes.

Switch libc.test.src.unistd.getopt_test to UNIT_TEST_ONLY.

5cc39f0

lntue force-pushed the test branch from a6c9030 to 5cc39f0 Compare January 26, 2024 15:59

lntue marked this pull request as ready for review January 26, 2024 16:04

gchatelet reviewed Jan 26, 2024

View reviewed changes

lntue added 2 commits January 26, 2024 11:54

Still use __internal__ target for hermetic tests on GPUs.

f1fc24b

Address comments and fix missing parentheses.

beaf40a

gchatelet mentioned this pull request Jan 26, 2024

[libc] Fix the api-test integration test #79627

Draft

Update integration tests and provide a global errno for GPU.

0f65a11

jhuber6 reviewed Jan 26, 2024

View reviewed changes

lntue added 6 commits January 27, 2024 12:05

Enable C symbol atexit for nVidia GPU target for testing.

fdd072d

Only set LIBC_COPT_PUBLIC_PACKAGING before atexit definition.

2d59b79

Another attempt at providing atexit C symbol for nVidia targets.

0b5ebb9

Fix missing LIBC_NAMESPACE.

1682b6a

Fix LibcTest target missing dependency and fix clock issues with tests.

ac46e45

Fix add_unittest_framework to link to the correct objects.

f592469

nickdesaulniers reviewed Jan 29, 2024

View reviewed changes

lntue mentioned this pull request Jan 31, 2024

[libc] Update libc_errno to work correctly in both overlay and full build modes. #80177

Merged

gchatelet mentioned this pull request Feb 15, 2024

[libc] Update integration tests to use the C functions directly #81866

Draft

[libc] Use LIBC_COPT_PUBLIC_PACKAGING for hermetic and integration tests. #79319

Are you sure you want to change the base?

[libc] Use LIBC_COPT_PUBLIC_PACKAGING for hermetic and integration tests. #79319

Uh oh!

Conversation

lntue commented Jan 24, 2024

Uh oh!

llvmbot commented Jan 24, 2024

Uh oh!

jhuber6 commented Jan 24, 2024

Uh oh!

lntue commented Jan 24, 2024

Uh oh!

lntue commented Jan 26, 2024

Uh oh!

gchatelet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lntue commented Jan 26, 2024

Uh oh!

jhuber6 commented Jan 26, 2024

Uh oh!

gchatelet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lntue commented Jan 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchatelet commented Feb 13, 2024

Uh oh!

jhuber6 commented Feb 13, 2024

Uh oh!

gchatelet commented Feb 13, 2024

Uh oh!

gchatelet commented Feb 29, 2024

Uh oh!

jhuber6 commented Feb 29, 2024

Uh oh!

gchatelet commented Feb 29, 2024

Uh oh!

Uh oh!

[libc] Use `LIBC_COPT_PUBLIC_PACKAGING` for hermetic and integration tests. #79319

[libc] Use `LIBC_COPT_PUBLIC_PACKAGING` for hermetic and integration tests. #79319