build: Inherit flags from rustc by mrkajetanp · Pull Request #1279 · rust-lang/cc-rs (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation119 Commits1 Checks27 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
Where applicable, detect which RUSTFLAGS were set for rustc and convert them into their corresponding cc flags in order to ensure consistent codegen across Rust and non-Rust modules.
Resolves #1254
It's not fully ready yet, so far I'm looking for some feedback.
The main question is that as is the implementation doesn't check whether CC supports a flag before passing it. I tried using theis_flag_supported(_inner) functions to filter them out but the build script was infinite looping so I thought it could be good to just ask here first.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have some feedback with the code style/lint:
The main question is that as is the implementation doesn't check whether CC supports a flag before passing it. I tried using the
is_flag_supported(_inner)functions to filter them out but the build script was infinite looping so I thought it could be good to just ask here first.
It's because in is_flag_supported we create a new cc::Build object and use it for detecting support.
https://docs.rs/cc/latest/src/cc/lib.rs.html#660
Because the rust flag is turned on by default, it causes infinite looping.
To fix this, you could use self.flags_supported, which is checked just before compilation without creating a new cc::Build `
https://docs.rs/cc/latest/src/cc/lib.rs.html#1909
To fix this, you could use
self.flags_supported, which is checked just before compilation without creating a newcc::Build`
Ah interesting, I can't push to self.flags_supported because that'd require a &mut self, so I could only do that from an explicit builder function, from Build::new() or from compile().
I'm calling add_inherited_rustflags just before the snippet you linked where is_flag_supported_inner is called, should I not be able to just call that function myself directly instead of pushing to the Vec?
I'm calling add_inherited_rustflags just before the snippet you linked where is_flag_supported_inner is called, should I not be able to just call that function myself directly instead of pushing to the Vec?
Yes in that case you can call is_flag_supported_inner directly
Hmm so I'm calling the new function like this:
// Add cc flags inherited from matching rustc flags
if self.inherit_rustflags {
self.add_inherited_rustflags(&mut cmd, &target)?;
}
for flag in self.flags_supported.iter() {
if self
.is_flag_supported_inner(flag, &cmd.path, &target)
.unwrap_or(false)
{
cmd.push_cc_arg((**flag).into());
}
}
and then inside the function even doing just this makes it infinite loop on creating the Tool for compiler inside is_flag_supported_inner.
let supported = self
.is_flag_supported_inner(&cc_flags[0], &cmd.path, &target)
.unwrap_or(false);
Looking at the code I do not really understand why is_flag_supported_inner doesn't infinite loop on the first snippet already, can you explain what's different about these two calls? Sorry, not very familiar with the codebase..
and then inside the function even doing just this makes it infinite loop on creating the Tool for compiler inside is_flag_supported_inner
Sorry, can you clarify on what function is is_flag_supported_inner being called in that trigger infinite loop?
Sorry, can you clarify on what function is is_flag_supported_inner being called in that trigger infinite loop?
Yep, I'm calling is_flag_supported_inner inside add_inherited_rustflags - that's the second snippet. add_inherited_rustflags is being called inside try_get_compiler - that's the first snippet.
Ah indeed you're right, now it makes sense - many thanks! :)
Collaborator
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting on this 🎉!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot better to me, thanks for doing the work! Only nits from my side now.
Should be good now I think :)
Collaborator
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I am happy about the direction and general code, just a few questions regarding panic, ergonomic and some simple optimization
There's a merge conflict, I'd fine with either a merge or rebase, whatever is convenient to you.
Collaborator
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This is the last review pass from me, and it mostly contains code suggestions that can be applied easily.
Once resolved I will merge it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix the pipeline
Where applicable, detect which RUSTFLAGS were set for rustc and convert them into their corresponding cc flags in order to ensure consistent codegen across Rust and non-Rust modules.
I squashed the changes into the original commit, don't like unnecessary commit pollution
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
bors added a commit to rust-lang-ci/rust that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
bors added a commit to rust-lang-ci/rust that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
bors added a commit to rust-lang-ci/rust that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
bors added a commit to rust-lang-ci/rust that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
try-job: dist-i686-linux
bors added a commit to rust-lang-ci/rust that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
poliorcetics pushed a commit to poliorcetics/rust that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
CI: Add LTO support to clang in dist-x86_64-linux
After rust-lang/cc-rs#1279, we attempt to pass -flto=thin to clang. In dist-x86_64-linux, we don't build clang with the LLVMgold.so library so this fails. This attempts to resolve this
First, pass the binutils plugin include directory to Clang, which will build the library
Second, this library depends on the version of libstdc++ that we built specifically. However, despite both the RPATH and LD_LIBRARY_PATH pointing to /rustroot/lib, we incorrectly resolve to the system libstdc++, which doesn't load.
# LD_DEBUG=libs,files
2219: file=libstdc++.so.6 [0]; needed by /rustroot/bin/../lib/LLVMgold.so [0]
2219: find library=libstdc++.so.6 [0]; searching
2219: search path=/rustroot/bin/../lib/../lib (RPATH from file /rustroot/bin/../lib/LLVMgold.so)
2219: trying file=/rustroot/bin/../lib/../lib/libstdc++.so.6
2219: search path=/usr/lib64/tls:/usr/lib64 (system search path)
2219: trying file=/usr/lib64/tls/libstdc++.so.6
2219: trying file=/usr/lib64/libstdc++.so.6Using LD_PRELOAD causes it to correctly load the library
I think this is probably not the most maintainable way to do this, so opening to see if this is desired and if there's a better way of doing this
bors added a commit to rust-lang-ci/rust that referenced this pull request