[libunwind][libc++][libc++abi] Add cross-compilation flags to tests #67201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
libc++
|
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-libunwind ChangesThere 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. However, some targets (e.g. RISC-V) may require additional flags beyond the basic --target= that is already added by the current infrastructure. When cross-compiling with CMake, the recommended approach is to define a toolchain file that specifies the necessary flags needed to compile for a given target. There are two interesting variables here that we should also be passing when building the test binaries:
This patch only adds these flags to the default libc++ configurations, but if the clang-cl/gcc configurations could also benefit from this I'll update the patch. This should be a possible alternative to https://reviews.llvm.org/D151056 and #65517 Full diff: https://github.com/llvm/llvm-project/pull/67201.diff 8 Files Affected:
diff --git a/libcxx/test/configs/llvm-libc++-shared.cfg.in b/libcxx/test/configs/llvm-libc++-shared.cfg.in
index 143b3b3feae1109..82e7129d8f166a0 100644
--- a/libcxx/test/configs/llvm-libc++-shared.cfg.in
+++ b/libcxx/test/configs/llvm-libc++-shared.cfg.in
@@ -7,10 +7,10 @@ config.substitutions.append(('%{flags}',
'-pthread' + (' -isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else '')
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
))
config.substitutions.append(('%{link_flags}',
- '-nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++'
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++'
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libcxx/test/configs/llvm-libc++-static.cfg.in b/libcxx/test/configs/llvm-libc++-static.cfg.in
index e866d4f52a0629d..3dc4af1c000d59f 100644
--- a/libcxx/test/configs/llvm-libc++-static.cfg.in
+++ b/libcxx/test/configs/llvm-libc++-static.cfg.in
@@ -7,10 +7,10 @@ config.substitutions.append(('%{flags}',
'-pthread' + (' -isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else '')
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
))
config.substitutions.append(('%{link_flags}',
- '-nostdlib++ -L %{lib} -lc++ -lc++abi'
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -nostdlib++ -L %{lib} -lc++ -lc++abi'
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libcxxabi/test/configs/llvm-libc++abi-merged.cfg.in b/libcxxabi/test/configs/llvm-libc++abi-merged.cfg.in
index c6fa4301b459ce9..861fc26baccaaf7 100644
--- a/libcxxabi/test/configs/llvm-libc++abi-merged.cfg.in
+++ b/libcxxabi/test/configs/llvm-libc++abi-merged.cfg.in
@@ -6,11 +6,11 @@ config.substitutions.append(('%{flags}',
'-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} %{maybe-include-libunwind} -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} %{maybe-include-libunwind} -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
'-I %{libcxx}/test/support -I %{libcxx}/src'
))
config.substitutions.append(('%{link_flags}',
- '-nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++ -pthread'
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++ -pthread'
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libcxxabi/test/configs/llvm-libc++abi-shared.cfg.in b/libcxxabi/test/configs/llvm-libc++abi-shared.cfg.in
index 6b69205da77c992..86f4e654140589b 100644
--- a/libcxxabi/test/configs/llvm-libc++abi-shared.cfg.in
+++ b/libcxxabi/test/configs/llvm-libc++abi-shared.cfg.in
@@ -7,10 +7,10 @@ config.substitutions.append(('%{flags}',
'-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} %{maybe-include-libunwind} -I %{libcxx}/test/support -I %{libcxx}/src -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS'
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} %{maybe-include-libunwind} -I %{libcxx}/test/support -I %{libcxx}/src -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS'
))
config.substitutions.append(('%{link_flags}',
- '-nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++ -lc++abi -pthread'
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -nostdlib++ -L %{lib} -Wl,-rpath,%{lib} -lc++ -lc++abi -pthread'
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libcxxabi/test/configs/llvm-libc++abi-static.cfg.in b/libcxxabi/test/configs/llvm-libc++abi-static.cfg.in
index 352e2c39586e1ea..b0e312d1c73fdee 100644
--- a/libcxxabi/test/configs/llvm-libc++abi-static.cfg.in
+++ b/libcxxabi/test/configs/llvm-libc++abi-static.cfg.in
@@ -7,10 +7,10 @@ config.substitutions.append(('%{flags}',
'-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} %{maybe-include-libunwind} -I %{libcxx}/test/support -I %{libcxx}/src -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS'
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} %{maybe-include-libunwind} -I %{libcxx}/test/support -I %{libcxx}/src -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS'
))
config.substitutions.append(('%{link_flags}',
- '-nostdlib++ -L %{lib} -lc++ -lc++abi -pthread'
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -nostdlib++ -L %{lib} -lc++ -lc++abi -pthread'
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libunwind/test/configs/llvm-libunwind-merged.cfg.in b/libunwind/test/configs/llvm-libunwind-merged.cfg.in
index 10650f7edf66a2f..19f312c87ba4050 100644
--- a/libunwind/test/configs/llvm-libunwind-merged.cfg.in
+++ b/libunwind/test/configs/llvm-libunwind-merged.cfg.in
@@ -25,10 +25,10 @@ config.substitutions.append(('%{flags}',
'-isysroot {}'.format(local_sysroot) if local_sysroot else ''
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{{include}} {}'.format(' '.join(compile_flags))
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{{include}} {}'.format(' '.join(compile_flags))
))
config.substitutions.append(('%{link_flags}',
- '-L %{{lib}} -Wl,-rpath,%{{lib}} -lc++ {}'.format(' '.join(link_flags))
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -L %{{lib}} -Wl,-rpath,%{{lib}} -lc++ {}'.format(' '.join(link_flags))
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libunwind/test/configs/llvm-libunwind-shared.cfg.in b/libunwind/test/configs/llvm-libunwind-shared.cfg.in
index 97185e57234ff7f..44ee6ab4d906765 100644
--- a/libunwind/test/configs/llvm-libunwind-shared.cfg.in
+++ b/libunwind/test/configs/llvm-libunwind-shared.cfg.in
@@ -24,10 +24,10 @@ config.substitutions.append(('%{flags}',
'-isysroot {}'.format(local_sysroot) if local_sysroot else ''
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{{include}} {}'.format(' '.join(compile_flags))
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{{include}} {}'.format(' '.join(compile_flags))
))
config.substitutions.append(('%{link_flags}',
- '-L %{{lib}} -Wl,-rpath,%{{lib}} -lunwind {}'.format(' '.join(link_flags))
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ -L %{{lib}} -Wl,-rpath,%{{lib}} -lunwind {}'.format(' '.join(link_flags))
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
diff --git a/libunwind/test/configs/llvm-libunwind-static.cfg.in b/libunwind/test/configs/llvm-libunwind-static.cfg.in
index fc6a18d057f3884..763b6d4fe1d4618 100644
--- a/libunwind/test/configs/llvm-libunwind-static.cfg.in
+++ b/libunwind/test/configs/llvm-libunwind-static.cfg.in
@@ -27,10 +27,10 @@ config.substitutions.append(('%{flags}',
'-isysroot {}'.format(local_sysroot) if local_sysroot else ''
))
config.substitutions.append(('%{compile_flags}',
- '-nostdinc++ -I %{{include}} {}'.format(' '.join(compile_flags))
+ '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{{include}} {}'.format(' '.join(compile_flags))
))
config.substitutions.append(('%{link_flags}',
- '%{{lib}}/libunwind.a {}'.format(' '.join(link_flags))
+ '@CMAKE_EXE_LINKER_FLAGS_INIT@ %{{lib}}/libunwind.a {}'.format(' '.join(link_flags))
))
config.substitutions.append(('%{exec}',
'%{executor} --execdir %T -- '
|
ldionne
reviewed
Sep 25, 2023
I think it would be good if all of them did it, the reason I didn't is that I limited it to the ones I can test locally. Seeing as there is probably CI coverage for all of the configs, should I try updating the remaining ones? |
Yes, I think it makes sense for all of them to use that variable. Can you confirm that this is basically empty all the time except if you're using a custom CMake toolchain file? |
Grepping through the CMake source shows that the variable could have an effect on the tests when targeting Android and Windows, but for other targets (Linux,macOS) it is a set to |
|
You could add it to all the |
arichardson
force-pushed
the
libcxx-xcompile
branch
from
72464ba
to
337df13
Compare
|
Updated all configs, hopefully CI agrees that this is correct. |
ldionne
reviewed
Oct 5, 2023
arichardson
force-pushed
the
libcxx-xcompile
branch
2 times, most recently
from
6fba1ce
to
ba9765d
Compare
|
This would look good to me, but we seem to be failing the Windows tests! |
Unfortunately I can't see what's going wrong there and I don't have an easy way of reproducing it. I'll try to add some debugging code to this PR in the coming days. |
|
You could try this: Then I think it would fail later and probably in a way that provides more information. |
arichardson
force-pushed
the
libcxx-xcompile
branch
from
ba9765d
to
03ee114
Compare
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. However, some targets (e.g. RISC-V) may require additional flags beyond the basic --target= that is already added by the current infrastructure. When cross-compiling with CMake, the recommended approach is to define a toolchain file that specifies the necessary flags needed to compile for a given target. There are two interesting variables here that we should also be passing when building the test binaries: - CMAKE_CXX_FLAGS_INIT defines the flags we need for compilation jobs and will specify flags such as -mabi= and -march= - CMAKE_EXE_LINKER_FLAGS_INIT defines the linker flags that are needed to cross-compile for a given target. When not cross-compiling these variables will generally be empty.
arichardson
force-pushed
the
libcxx-xcompile
branch
from
e803801
to
041c025
Compare
EricWF
requested changes
Oct 20, 2023
| @@ -45,10 +45,10 @@ config.substitutions.append(('%{flags}', | |||
| '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else '' | |||
| )) | |||
| config.substitutions.append(('%{compile_flags}', | |||
| '-nostdinc++ -I %{include} -I %{libcxx}/test/support' | |||
| '@CMAKE_CXX_FLAGS_INIT@ -nostdinc++ -I %{include} -I %{libcxx}/test/support' | |||
There was a problem hiding this comment.
I don't think this makes sense. The flags you use to build the library may not be the flags our users use when using it.
I think the test suite should handle its flags manually, and we should find another route to get you what you want here.
There was a problem hiding this comment.
My thinking here was that CMAKE_CXX_FLAGS_INIT are kind of like flags specified in a toolchain file -- they represent flags that literally any program compiled for target X needs to be passed. In that sense, they would be required for both the library and the test suite.
For instance, @arichardson don't you need to pass -mabi= or -march= to both the library and the test suite? I am still thinking that the proper way to actually do this would be to create .cfg.in config files that pass the target flags you need. Or if you need to specify various values for e.g. -mabi, you can create a Lit parameter that is local to that configuration like we do in libcxx/test/configs/apple-libc++-backdeployment.cfg.in. It feels like I've suggested that in the past though, so maybe there's a reason why you didn't go down that road?
|
We also don't want to make our test suite flags beholden to changes CMake chooses to make. |
|
I'll take another pass at this on the weekend so I can provide some concrete direction. |
|
Debugging shows that the reason this is failing on the clang-cl bots is that the CMAKE_CXX_FLAGS_INIT are using the MSVC syntax but we pass --driver-mode=g++ explicitly: |
I am less convinced now that this is the right solution since the CMake flags refer to the library build rather than the testing configuration, so maybe an explicit test suite parameter as in #65517 is the cleaner solution? |
|
Closing since it seems we don't want to pursue this anymore, since |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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. However, some targets (e.g. RISC-V) may require additional flags beyond the basic --target= that is already added by the current infrastructure.
When cross-compiling with CMake, the recommended approach is to define a toolchain file that specifies the necessary flags needed to compile for a given target. There are two interesting variables here that we should also be passing when building the test binaries:
This patch only adds these flags to the default libc++ configurations, but if the clang-cl/gcc configurations could also benefit from this I'll update the patch.
This should be a possible alternative to https://reviews.llvm.org/D151056 and #65517