-
Notifications
You must be signed in to change notification settings - Fork 21
Fix Cross-compile Linking Args #26
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
Conversation
|
Verified on morty that these arguments now mean the |
generate-clang-meson-cross-file.sh
Outdated
| c_link_args = [${meson_cflags}] | ||
| cpp_args = [${meson_cflags}] | ||
| cpp_link_args = [${meson_cflags}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This concerns me a bit. It's true that some compilation options also need to be passed as link options, but others don't make sense. Is there a reason for why meson_cflags will always contain options that make sense for both compilation and linking? If not, can't we further separate this?
BTW, can you specify which options were missing? That might shed some light on the best way forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been very careful to ensure only toolchain-related flags end up in meson_cflags.
What is a toolchain-related flag? One that is needed to ensure the compiler always compiles in a way that is compatible with the toolchain, or to ensure it uses the toolchain. This list currently is
-march-mabi-mcmodel--gcc-toolchain(so clang does lookup correctly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was already assuming that you had done a good job with such separation :-) My point was that I was concerned that, given the name meson_cflags, it wouldn't be clear in the future. This was coming from the perspective of CFLAGS and LDFLAGS naming, where the CFLAGS content isn't necessarily applicable to LDFLAGS, so I was concerned that it wouldn't be clear that you couldn't add stuff to meson_cflags that would also apply to the linking stage. If you think this isn't going to be a problem or that there isn't an easy way to address this potential pitfall, then that's good enough :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see where you're going with this. Maybe we should call it meson_driver_flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Signed-off-by: Sam Elliott <selliott@lowrisc.org>
This is a new feature of meson v0.53.1 which chooses the linker in a language-specific manner. We just set the linker for C and C++ to explicitly be riscv32-unknown-elf-ld, the linker we normally use. In the clang builds, this will be passed with `clang -fuse-ld=<path>` which should work correctly, but uses the clang driver to establish all the right arguments to pass down to the linker. Signed-off-by: Sam Elliott <selliott@lowrisc.org>
a5d052e to
8da2bc7
Compare
luismarques
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I was seeing some path resolution issues when using the clang cross file on a system that had a system install of the risc-v toolchain. Debugging this further, it seems like we need to pass these toolchain arguments down to
<lang>_link_argsas well as<lang>_argsso that the clang driver picks up the right libraries to use.Additionally, meson v0.53.1 introduced some per-language linker specification options, so I'm protecting us from getting bitten by that by explicitly setting those to the same as the
ldoption.