exclude #![no_builtins] crates from LTO
#35637
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
this prevents intrinsics like `memcpy` from being mis-optimized to infinite recursive calls when LTO is used. fixes rust-lang#31544 closes rust-lang#35540
japaric
force-pushed
the
no-builtins-lto
branch
from
ed0fef5 to
db16909
Compare
| @@ -938,8 +938,10 @@ fn add_upstream_rust_crates(cmd: &mut Linker, | |||
| Linkage::NotLinked | | |||
| Linkage::IncludedFromDylib => {} | |||
| Linkage::Static => { | |||
| let is_a_no_builtins_crate = | |||
| attr::contains_name(&sess.cstore.crate_attrs(cnum), "no_builtins"); | |||
There was a problem hiding this comment.
Could this logic for checking this attribute be centralized somewhere? I think there's probably at least one other instance of this check (besides the two added here) which reads that in the first place as well.
There was a problem hiding this comment.
I've seen an CrateTranslation struct that contains a no_builtins field. I'll see if I can pass it around in these contexts.
There was a problem hiding this comment.
So that struct is not really usable because in all these contexts the only thing we know about the dependency is its CrateNum (basically an ID number) and there's no way to get a CrateTranslation from it. Even if that were possible it would be more expensive, I think, that the current attr::contains_name call. However, an improvement we could do is to change this attr::contains_name call for something more readable like sess.cstore.is_no_builtins_crate(cnum) everywhere.
There was a problem hiding this comment.
Ah yeah something more readable is fine, I'd just want to deduplicate the string literal "no_builtins"
|
Looks good to me, thanks @japaric! Could you also add a test for this in-tree? |
I think that's going to be tricky. I have only seen the misoptimization when building an ARM executable that uses the |
|
Ah yeah it's probably fine to not actually test for something (as that would be really hard), but it'd be nice to have a crate which at least exercises the code paths perhaps? |
|
Another possibility would be to inspect the output of |
|
@alexcrichton OK. Did the CStore refactor and added the print-link-args test. |
|
Looks good to me! I'm not entirely sure if that test will pass on MSVC, but we'll find out. In the meantime though I think a binary was checked in by accident which is causing tidy to fail. Other than that though r=me |
japaric
force-pushed
the
no-builtins-lto
branch
from
e9ee1e9 to
e996405
Compare
|
Edited the last commit to remove the binary. @bors r=alexcrichton |
|
📌 Commit e996405 has been approved by |
exclude `#![no_builtins]` crates from LTO this prevents intrinsics like `memcpy` from being mis-optimized to infinite recursive calls when LTO is used. fixes #31544 closes #35540 --- r? @alexcrichton cc @Amanieu
…, r=pnkfelix Add the `no-builtins` attribute to functions when `no_builtins` is applied at the crate level. **When `no_builtins` is applied at the crate level, we should add the `no-builtins` attribute to each function to ensure it takes effect in LTO.** This is also the reason why no_builtins does not take effect in LTO as mentioned in rust-lang#35540. Now, `#![no_builtins]` should be similar to `-fno-builtin` in clang/gcc, see https://clang.godbolt.org/z/z4j6Wsod5. Next, we should make `#![no_builtins]` participate in LTO again. That makes sense, as LTO also takes into consideration function-level instruction optimizations, such as the MachineOutliner. More importantly, when a user writes a large `#![no_builtins]` crate, they would like this crate to participate in LTO as well. We should also add a function-level no_builtins attribute to allow users to have more control over it. This is similar to Clang's `__attribute__((no_builtin))` feature, see https://clang.godbolt.org/z/Wod6KK6eq. Before implementing this feature, maybe we should discuss whether to support more fine-grained control, such as `__attribute__((no_builtin("memcpy")))`. Related discussions: - rust-lang#109821 - rust-lang#35540 Next (a separate pull request?): - [ ] Revert rust-lang#35637 - [ ] Add a function-level `no_builtin` attribute?
…, r=pnkfelix Add the `no-builtins` attribute to functions when `no_builtins` is applied at the crate level. **When `no_builtins` is applied at the crate level, we should add the `no-builtins` attribute to each function to ensure it takes effect in LTO.** This is also the reason why no_builtins does not take effect in LTO as mentioned in rust-lang#35540. Now, `#![no_builtins]` should be similar to `-fno-builtin` in clang/gcc, see https://clang.godbolt.org/z/z4j6Wsod5. Next, we should make `#![no_builtins]` participate in LTO again. That makes sense, as LTO also takes into consideration function-level instruction optimizations, such as the MachineOutliner. More importantly, when a user writes a large `#![no_builtins]` crate, they would like this crate to participate in LTO as well. We should also add a function-level no_builtins attribute to allow users to have more control over it. This is similar to Clang's `__attribute__((no_builtin))` feature, see https://clang.godbolt.org/z/Wod6KK6eq. Before implementing this feature, maybe we should discuss whether to support more fine-grained control, such as `__attribute__((no_builtin("memcpy")))`. Related discussions: - rust-lang#109821 - rust-lang#35540 Next (a separate pull request?): - [ ] Revert rust-lang#35637 - [ ] Add a function-level `no_builtin` attribute?
…, r=pnkfelix Add the `no-builtins` attribute to functions when `no_builtins` is applied at the crate level. **When `no_builtins` is applied at the crate level, we should add the `no-builtins` attribute to each function to ensure it takes effect in LTO.** This is also the reason why no_builtins does not take effect in LTO as mentioned in rust-lang#35540. Now, `#![no_builtins]` should be similar to `-fno-builtin` in clang/gcc, see https://clang.godbolt.org/z/z4j6Wsod5. Next, we should make `#![no_builtins]` participate in LTO again. That makes sense, as LTO also takes into consideration function-level instruction optimizations, such as the MachineOutliner. More importantly, when a user writes a large `#![no_builtins]` crate, they would like this crate to participate in LTO as well. We should also add a function-level no_builtins attribute to allow users to have more control over it. This is similar to Clang's `__attribute__((no_builtin))` feature, see https://clang.godbolt.org/z/Wod6KK6eq. Before implementing this feature, maybe we should discuss whether to support more fine-grained control, such as `__attribute__((no_builtin("memcpy")))`. Related discussions: - rust-lang#109821 - rust-lang#35540 Next (a separate pull request?): - [ ] Revert rust-lang#35637 - [ ] Add a function-level `no_builtin` attribute?
…, r=pnkfelix Add the `no-builtins` attribute to functions when `no_builtins` is applied at the crate level. **When `no_builtins` is applied at the crate level, we should add the `no-builtins` attribute to each function to ensure it takes effect in LTO.** This is also the reason why no_builtins does not take effect in LTO as mentioned in rust-lang#35540. Now, `#![no_builtins]` should be similar to `-fno-builtin` in clang/gcc, see https://clang.godbolt.org/z/z4j6Wsod5. Next, we should make `#![no_builtins]` participate in LTO again. That makes sense, as LTO also takes into consideration function-level instruction optimizations, such as the MachineOutliner. More importantly, when a user writes a large `#![no_builtins]` crate, they would like this crate to participate in LTO as well. We should also add a function-level no_builtins attribute to allow users to have more control over it. This is similar to Clang's `__attribute__((no_builtin))` feature, see https://clang.godbolt.org/z/Wod6KK6eq. Before implementing this feature, maybe we should discuss whether to support more fine-grained control, such as `__attribute__((no_builtin("memcpy")))`. Related discussions: - rust-lang#109821 - rust-lang#35540 Next (a separate pull request?): - [ ] Revert rust-lang#35637 - [ ] Add a function-level `no_builtin` attribute?
this prevents intrinsics like
memcpyfrom being mis-optimized toinfinite recursive calls when LTO is used.
fixes #31544
closes #35540
r? @alexcrichton
cc @Amanieu