Skip to content

[libc] Add ${CMAKE_CROSSCOMPILING_EMULATOR} to custom test cmdlines #66565

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 22, 2023
Merged

[libc] Add ${CMAKE_CROSSCOMPILING_EMULATOR} to custom test cmdlines #66565

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
872765d
[libc] Add ${CMAKE_CROSSCOMPILING_EMULATOR} to custom test cmdlines
mikhailramalho Sep 16, 2023
64b0e3c
Put CMAKE_CROSSCOMPILING_EMULATOR right after the gpu_loader_exe
mikhailramalho Sep 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libc/cmake/modules/LLVMLibCTestRules.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ function(add_integration_test test_name)
# to expand the list (by including the option COMMAND_EXPAND_LISTS). This
# makes `add_custom_target` construct the correct command and execute it.
set(test_cmd
${INTEGRATION_TEST_ENV}
${CMAKE_CROSSCOMPILING_EMULATOR} ${INTEGRATION_TEST_ENV}
Copy link
Contributor

@jhuber6 jhuber6 Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do the following for the GPU build.

$> clang main.c --target=amdgcn-amd-amdhsa -mcpu=gfx1030 crt1.o -lc -o exe
$> ./emulator <optional arguments> exe

This looks very similar to cross-compiling by design. I'm thinking it would be good to unify this approach with the GPU build. This is just me thinking out loud.

Copy link
Member Author

@mikhailramalho mikhailramalho Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea, but how do you handle env variables in these cases?

Currently, the script I wrote expects <env variables> <full path to binary> <args> to work properly... We need the full path to the binary to scp any test data present in the binary directory; we then copy the binary to a temp dir in the qemu-system and call ssh foo@bar <env variables> ~/<temp dir>/<binary> <args>.

(I should probably upload the script I'm using somewhere).

Copy link
Contributor

@jhuber6 jhuber6 Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use any env vars, but presumably it would be handled just by the loader. E.g. if I do env foo ./amdhsa_loader <args> <image> it would read foo from the environment and pass it to the image it launches.

Copy link
Member Author

@mikhailramalho mikhailramalho Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know does it detects the foo env variable in the loader? Or will it just send every env var on the system to the image?

Copy link
Contributor

@jhuber6 jhuber6 Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every single one, it clones the current user environment.

Copy link
Contributor

@jhuber6 jhuber6 Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/llvm/llvm-project/blob/main/libc/utils/gpu/loader/Loader.h#L92 here's the code for it, pretty much just uses the envp passed in from the main function.

Copy link
Member Author

@mikhailramalho mikhailramalho Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that actually makes things easier for me.

What do you think about dropping gpu_loader_exe and using just CMAKE_CROSSCOMPILING_EMULATOR instead?

Copy link
Contributor

@jhuber6 jhuber6 Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be an argument passed in the cmake invocation? Because the gpu_loader_exe is built internally so it's not known until CMake runs. We could potentially just set CMAKE_CROSSCOMPILING_EMULATOR manually inside of libc? Maybe that would override something the user set, unsure.

Copy link
Member Author

@mikhailramalho mikhailramalho Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran some tests here and it works both ways: either you set it during the cmake config via cmd line or you can set it using set(CMAKE_CROSSCOMPILING_EMULATOR "foobar").

My only concern is that it doesn't tell the user that it overwrote an argument passed via cmd line, but I guess we can always throw a warning there.

I'll update this PR to put $CMAKE_CROSSCOMPILING_EMULATOR right after the $gpu_loader_exe for now, and send all environment variables as the loader does. If you can test using only $CMAKE_CROSSCOMPILING_EMULATOR and it does work, we can send another PR to remove $gpu_loader_exe later.

Copy link
Contributor

@jhuber6 jhuber6 Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG, I'll test it once this lands.

$<$<BOOL:${LIBC_TARGET_ARCHITECTURE_IS_GPU}>:${gpu_loader_exe}>
${INTEGRATION_TEST_LOADER_ARGS}
$<TARGET_FILE:${fq_build_target_name}> ${INTEGRATION_TEST_ARGS})
Expand Down Expand Up @@ -728,7 +728,7 @@ function(add_libc_hermetic_test test_name)

set(test_cmd ${HERMETIC_TEST_ENV}
$<$<BOOL:${LIBC_TARGET_ARCHITECTURE_IS_GPU}>:${gpu_loader_exe}> ${HERMETIC_TEST_LOADER_ARGS}
$<TARGET_FILE:${fq_build_target_name}> ${HERMETIC_TEST_ARGS})
${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:${fq_build_target_name}> ${HERMETIC_TEST_ARGS})
add_custom_target(
${fq_target_name}
COMMAND ${test_cmd}
Expand Down