This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add rule named `add_libc_hermetic_test` which adds a hermetic test.
ClosedPublic

Authored by sivachandra on Apr 19 2023, 4:25 PM.

Details

Summary

A convenience wrapper name add_libc_test is also added which adds both
a unit test and a hermetic test. The ctype tests have been switched over
to use add_libc_test.

Diff Detail

Event Timeline

sivachandra created this revision.Apr 19 2023, 4:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 19 2023, 4:25 PM
sivachandra requested review of this revision.Apr 19 2023, 4:25 PM

I am sharing this early so that other can try it out. It is not in the shape I want to land so will be sending out more updates.

jhuber6 added inline comments.Apr 19 2023, 8:08 PM
libc/cmake/modules/LLVMLibCTestRules.cmake
629

This is a typo I just fixed upstream.

679

Small change here just today that adds LOADER_ARGS to support calling tests with multiple threads on the GPU.

Thanks a lot for this work, I think it's getting us pretty close. Getting this to build for the GPU required adding the UnitTest and src directories in the CMake. Some other issues I've had when trying to build for the GPU.

  1. Including stdlib.h in src/__support/CPP/string.h causes errors when they include the system headers. Presumably this is because we aren't providing this as a header in the GPU build yet?
  2. The calls to ::free and ::realloc in src/__support/CPP/string.h did not compile when I tried, they were undefined in the module.
  3. I got an LLVM crash when compiling the whole project targeting AMDGPU in LTO mode. This is the recommended way as AMDGPU doesn't have a true "ABI". Turning off LTO hides the crash but is a little more iffy on whether or not it's correct since there's no defined ABI for cross-TU calls.
  4. My build was not picking up libc.src.__support.OSUtil.osutil in the final test for some reason, added it to the hermetic test and it worked.
  5. We do not have __cxa_atexit defined
  6. The UnitTest libraries need to have the explicit --target= and -mcpu= options passed in all cases to match the source.
  7. After hacking around all of the above, the test built and runs with a "No tests run." message. I'm assuming this is because we are not handling the global constructors.

I'd appreciate some pointers on how to fix some of the above on the GPU side, I'm not as intimately familiar with the libc internals for registering constructors / exit functions.

libc/cmake/modules/LLVMLibCTestRules.cmake
568

Optional LOADER_ARGS here needed for the GPU.

679
libc/test/UnitTest/CMakeLists.txt
14

The GPU support will require overloading the triple and architecture for each one of these. So the above should work applied as compile options for each add_library under here.

Pass hermetic test compile args to the hermetic test framework also.

sivachandra marked 4 inline comments as done.

Add doc string for LOADER_ARGS.

Thanks a lot for this work, I think it's getting us pretty close. Getting this to build for the GPU required adding the UnitTest and src directories in the CMake. Some other issues I've had when trying to build for the GPU.

  1. Including stdlib.h in src/__support/CPP/string.h causes errors when they include the system headers. Presumably this is because we aren't providing this as a header in the GPU build yet?

I think so.

  1. The calls to ::free and ::realloc in src/__support/CPP/string.h did not compile when I tried, they were undefined in the module.

Did not compile or did they produce linker errors? The allocator functions come from the hermetic test framework, just like with the integration test framework.

  1. I got an LLVM crash when compiling the whole project targeting AMDGPU in LTO mode. This is the recommended way as AMDGPU doesn't have a true "ABI". Turning off LTO hides the crash but is a little more iffy on whether or not it's correct since there's no defined ABI for cross-TU calls.

Is this related to the libc in anyway?

  1. My build was not picking up libc.src.__support.OSUtil.osutil in the final test for some reason, added it to the hermetic test and it worked.

Likely a missing dep somewhere? But, this patch does add OSUtil.osutil as a dep.

  1. We do not have __cxa_atexit defined

This comes from the atexit implementation. This should be enabled for GPUs in some mannger.

  1. The UnitTest libraries need to have the explicit --target= and -mcpu= options passed in all cases to match the source.

I have updated this patch to get all the relevant compile options.

  1. After hacking around all of the above, the test built and runs with a "No tests run." message. I'm assuming this is because we are not handling the global constructors.

I'd appreciate some pointers on how to fix some of the above on the GPU side, I'm not as intimately familiar with the libc internals for registering constructors / exit functions.

Likely. I am happy to help/talk about how this works with normal ELF CPU binaries.

Thanks a lot for this work, I think it's getting us pretty close. Getting this to build for the GPU required adding the UnitTest and src directories in the CMake. Some other issues I've had when trying to build for the GPU.

  1. Including stdlib.h in src/__support/CPP/string.h causes errors when they include the system headers. Presumably this is because we aren't providing this as a header in the GPU build yet?

I think so.

I should probably enable a "malloc" that just allocates from a fixed size pool like we do in the integration tests, better than nothing.

  1. The calls to ::free and ::realloc in src/__support/CPP/string.h did not compile when I tried, they were undefined in the module.

Did not compile or did they produce linker errors? The allocator functions come from the hermetic test framework, just like with the integration test framework.

Didn't compile, it was a C++ thing.

  1. I got an LLVM crash when compiling the whole project targeting AMDGPU in LTO mode. This is the recommended way as AMDGPU doesn't have a true "ABI". Turning off LTO hides the crash but is a little more iffy on whether or not it's correct since there's no defined ABI for cross-TU calls.

Is this related to the libc in anyway?

Probably not, just noting that it'll be a roadblock. Once this support is in fully I'll create a reproducer and see if I can get it fixed.

  1. My build was not picking up libc.src.__support.OSUtil.osutil in the final test for some reason, added it to the hermetic test and it worked.

Likely a missing dep somewhere? But, this patch does add OSUtil.osutil as a dep.

It used to be in the add_integration_test but that wasn't copied over to the hermetic test when I looked at it.

  1. We do not have __cxa_atexit defined

This comes from the atexit implementation. This should be enabled for GPUs in some mannger.

Alright, I'll need to make a version that eschews the mutexes most likely.

  1. After hacking around all of the above, the test built and runs with a "No tests run." message. I'm assuming this is because we are not handling the global constructors.

I'd appreciate some pointers on how to fix some of the above on the GPU side, I'm not as intimately familiar with the libc internals for registering constructors / exit functions.

Likely. I am happy to help/talk about how this works with normal ELF CPU binaries.

That sounds good, we can set up a time if you're free.

I should probably enable a "malloc" that just allocates from a fixed size pool like we do in the integration tests, better than nothing.

This patch adds a HermeticTestUtils.cpp which does that.

  1. The calls to ::free and ::realloc in src/__support/CPP/string.h did not compile when I tried, they were undefined in the module.

Did not compile or did they produce linker errors? The allocator functions come from the hermetic test framework, just like with the integration test framework.

Didn't compile, it was a C++ thing.

May be this: https://reviews.llvm.org/D149075

Alright, I'll need to make a version that eschews the mutexes most likely.

We need pass-through mutexes for other contexts also. So, we should have a more generic no-mutex solution.

jhuber6 accepted this revision.Apr 24 2023, 10:24 AM

No objections from me. Still a lot of work required to get it working on the GPU, but this is a very good step. Thanks.

libc/cmake/modules/LLVMLibCTestRules.cmake
648

Leftover?

This revision is now accepted and ready to land.Apr 24 2023, 10:24 AM
sivachandra added inline comments.Apr 24 2023, 3:49 PM
libc/cmake/modules/LLVMLibCTestRules.cmake
648

Oops. I missed this comment. I have a few follow up changes so I will fix it in those patches.