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.

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

Conversation

mikhailramalho
Copy link
Member

${CMAKE_CROSSCOMPILING_EMULATOR} will be used in the new rv32 buildbot and is prepended automatically when we call add_custom_target in CMake, except when we use a custom command.

There are two places where custom commands are used in libc, so we explicitly add the ${CMAKE_CROSSCOMPILING_EMULATOR} variable there. Other systems that don't use ${CMAKE_CROSSCOMPILING_EMULATOR} are unaffected

${CMAKE_CROSSCOMPILING_EMULATOR} will be used in the new rv32 buildbot
and is preppended automatically when we call add_custom_target in CMake,
except when we use a custom command.

There are two places where custom commands are used in libc, so we
explicitly add the ${CMAKE_CROSSCOMPILING_EMULATOR} variable there.
Other systems that don't use ${CMAKE_CROSSCOMPILING_EMULATOR} are
unaffected
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2023

@llvm/pr-subscribers-libc

Changes

${CMAKE_CROSSCOMPILING_EMULATOR} will be used in the new rv32 buildbot and is prepended automatically when we call add_custom_target in CMake, except when we use a custom command.

There are two places where custom commands are used in libc, so we explicitly add the ${CMAKE_CROSSCOMPILING_EMULATOR} variable there. Other systems that don't use ${CMAKE_CROSSCOMPILING_EMULATOR} are unaffected

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

1 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCTestRules.cmake (+2-2)
diff --git a/libc/cmake/modules/LLVMLibCTestRules.cmake b/libc/cmake/modules/LLVMLibCTestRules.cmake
index e1286b4ab963189..552cbd739628deb 100644
--- a/libc/cmake/modules/LLVMLibCTestRules.cmake
+++ b/libc/cmake/modules/LLVMLibCTestRules.cmake
@@ -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}
       $<$<BOOL:${LIBC_TARGET_ARCHITECTURE_IS_GPU}>:${gpu_loader_exe}>
       ${INTEGRATION_TEST_LOADER_ARGS}
       $<TARGET_FILE:${fq_build_target_name}> ${INTEGRATION_TEST_ARGS})
@@ -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}

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

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

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

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

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

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

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

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

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

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

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.

@mikhailramalho mikhailramalho merged commit 50d1500 into llvm:main Sep 22, 2023
@mikhailramalho mikhailramalho deleted the add-cross-emul branch September 22, 2023 13:45
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Sep 28, 2023
Local branch amd-gfx aa1d706 Merged main:eb02ee44d325 into amd-gfx:0cc6e26f9eb2
Remote branch main 50d1500 [libc] Add ${CMAKE_CROSSCOMPILING_EMULATOR} to custom test cmdlines (llvm#66565)
@nickdesaulniers
Copy link
Member

Do you have any instructions on how this was used? I'd like to try to revive some testing via qemu.

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.

6 participants