Skip to content

Conversation

arichardson
Copy link
Member

There is currently only limited support for passing target-specific flags (e.g. -mabi= or -march=) to the testsuites if you don't have a compiler (or wrapper script) that defaults to the expected flags.

While there are parameters to pass --target= and
-isysroot (https://reviews.llvm.org/D151056 tries to extend that to --sysroot= so the linker can find libraries), building for some targets e.g. RISC-V may require additional flags when building with Clang.

This change adds a new target_flags parameter to the testsuite that appends the given flags to %{flags} and can be set from CMake using {LIBUNWIND,LIBCXXABI_LIBCXX}_TEST_TARGET_FLAGS.

There is currently only limited support for passing target-specific
flags (e.g. -mabi= or -march=) to the testsuites if you don't have a
compiler (or wrapper script) that defaults to the expected flags.

While there are parameters to pass --target= and
-isysroot (https://reviews.llvm.org/D151056 tries to extend that to
--sysroot= so the linker can find libraries), building for some targets
e.g. RISC-V may require additional flags when building with Clang.

This change adds a new target_flags parameter to the testsuite that
appends the given flags to %{flags} and can be set from CMake using
`{LIBUNWIND,LIBCXXABI_LIBCXX}_TEST_TARGET_FLAGS.`
@arichardson arichardson requested review from a team as code owners September 6, 2023 18:53
@github-actions github-actions bot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libunwind libc++-abi labels Sep 6, 2023
@arichardson
Copy link
Member Author

I am also considering adding parameters for %{compile_flags} and %{link_flags} that could be defaulted to the values commonly set by CMake cross-compilation toolchain files. E.g. something like the following

if (CMAKE_CXX_FLAGS_INIT)
  serialize_lit_param(target_compile_flags "\"${CMAKE_CXX_FLAGS_INIT}\"")
endif()
if (CMAKE_EXE_LINKER_FLAGS_INIT)
  serialize_lit_param(target_link_flags "\"${CMAKE_EXE_LINKER_FLAGS_INIT}\"")
endif()

@ldionne ldionne self-assigned this Sep 13, 2023
@ldionne ldionne added the wontfix Issue is real, but we can't or won't fix it. Not invalid label Sep 13, 2023
@ldionne
Copy link
Member

ldionne commented Sep 13, 2023

The direction we've been taking with these config files is to keep the base config setup as simple as possible and avoid the proliferation of options that are not fundamentally tied to the test suite. These kinds of options should instead be hardcoded in a suitable .cfg.in file for the platform you're trying to test. So instead of trying to set -mabi= here, you should create a riscv config file that describes how to build and run the test suite on that platform. This constrains the platform complexity to a single file and avoids leaking into the rest of the test suite.

I'll tentatively close this for now based on the above reasoning, but please reopen for discussion if you disagree.

@ldionne ldionne closed this Sep 13, 2023
@arichardson
Copy link
Member Author

The problem is that we end up with a combinatoric explosion of config files for all the different ABIs that can be used.

Would you be open to having the cmake-bridge inject CMAKE_CXX_FLAGS_INIT/CMAKE_EXE_LINKER_FLAGS_INIT (which should be empty unless you use a toolchain file).

@ldionne
Copy link
Member

ldionne commented Sep 13, 2023

Your .cfg.in files can use CMake variables themselves! In fact you can even add new parameters that are only valid for that configuration of the library, for example see https://github.com/llvm/llvm-project/blob/main/libcxx/test/configs/apple-libc%2B%2B-backdeployment.cfg.in#L14-L42

Basically, everyone has their funky use case for testing the library, and I am trying to avoid adding complexity to the "main" code path every time someone has a new use case. These config files allow hiding that complexity away from the main code path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libunwind wontfix Issue is real, but we can't or won't fix it. Not invalid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants