[libc-commits] [PATCH] D148756: [libc] Add rule named `add_libc_hermetic_test` which adds a hermetic test.

Mon Apr 24 06:30:15 PDT 2023

jhuber6 added a comment.

In D148756#4291622 <https://reviews.llvm.org/D148756#4291622>, @sivachandra wrote:

> In D148756#4283369 <https://reviews.llvm.org/D148756#4283369>, @jhuber6 wrote:
>
>> 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.

>> 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.
>
> 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.

>> 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.
>
> 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.

>> 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.
>
> 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.

>> 5. 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.

>> 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.
>
> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148756/new/

https://reviews.llvm.org/D148756