Add the no-builtins attribute to functions when no_builtins is applied at the crate level.

[DianQK]

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 #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:

• Fixes LTO + build-std + Oz failed to resolve undefined symbols #109821
• #![no_builtins] should prevent a crate from participating in LTO #35540

Next (a separate pull request?):

• Revert exclude #![no_builtins] crates from LTO #35637
• Add a function-level no_builtin attribute?

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @b-naber (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[DianQK]

r? pnkfelix

[bjorn3]

#113684 accidentally got merged due to a glitch in bors. We had to force push it away. It seems that you based this PR on the state of the repo when that PR was still merged. Could you please rebase and drop these commits?

[DianQK]

@bjorn3 Are there any outstanding tasks to be completed for this pull request? I think I can revert #35637.

[pnkfelix]

If I understand correctly, this PR represents a step towards resolving #72140, but this PR on its own won't resolve that issue.

Rather, the plan is: 1. land this PR (PR #113716), and then 2. revert the code change of PR #35637, and we believe that will yield a working overall state that also has issue #72140 resolved. Does that all sound correct, @DianQK ?

(I was going to say something like "revert the code change of PR #35637 while keeping its test intact, but looking at the test added in that PR, I'm not sure it represents end-to-end behavior we are too concerned about preserving in terms of the details of what linker-invocation is generated. Rather, the more important thing is to check that examples like the ones from https://github.com/DianQK/rust-109821-reproducible/ (see also #110606) start passing under the plan outlined here, I think...)

[DianQK]

Rather, the plan is: 1. land this PR (PR #113716), and then 2. revert the code change of PR #35637, and we believe that will yield a working overall state that also has issue #72140 resolved. Does that all sound correct, @DianQK ?

Yes, I think so.

[pnkfelix]

@bors r+

[bors]

📌 Commit 063ddbe has been approved by pnkfelix

It is now in the queue for this repository.

[pnkfelix]

@bors rollup

[DianQK]

(I was going to say something like "revert the code change of PR #35637 while keeping its test intact, but looking at the test added in that PR, I'm not sure it represents end-to-end behavior we are too concerned about preserving in terms of the details of what linker-invocation is generated. Rather, the more important thing is to check that examples like the ones from https://github.com/DianQK/rust-109821-reproducible/ start passing under the plan outlined here, I think...)

I think the #35637 test is just to verify the exclusion of crates with the attribute #![no_builtins] from LTO.
We can delete it directly.

I am uncertain about the necessity of the test case from https://github.com/DianQK/rust-109821-reproducible/. In my opinion, it appears to be unnecessary.

[pnkfelix]

I am uncertain about the necessity of the test case from https://github.com/DianQK/rust-109821-reproducible/. In my opinion, it appears to be unnecessary.

Well, we should do something to check our capability to combine #![no_builtins] with LTO. After all, after you've put in all this effort to find a path to enable us to support that combination of features, we should put some kind of regression test into our test suite. I just figured your test (or something similar) would be the easiest thing to start from.

[DianQK]

Well, we should do something to check our capable to combine #![no_builtins] with LTO. After all, after you've put in all this effort to find a path to enable us to support that combination of features, we should put some kind of regression test into our test suite. I just figured your test (or something similar) would be the easiest thing to start from.

It appears that @bors is in the process of merging this PR. I might consider submitting another PR to revert #35637, along with an appropriate test case to be considered.

[pnkfelix]

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"))).

FYI these kinds of changes may need a T-lang design proposal or T-compiler Major Change Proposal.

The proposals may be easy to adopt, but its still good to put them through the discussion process, rather than go straight to putting them up as PR's, unless you personally prefer to try to express your design ideas via PR's (but even then, a lot of people prefer to read design ideas in a format that is not a low-level PR.)

[pnkfelix]

Well, we should do something to check our capable to combine #![no_builtins] with LTO. After all, after you've put in all this effort to find a path to enable us to support that combination of features, we should put some kind of regression test into our test suite. I just figured your test (or something similar) would be the easiest thing to start from.

It appears that @bors is in the process of merging this PR. I might consider submitting another PR to revert #35637, along with an appropriate test case to be considered.

Oh, I'm sorry, I wasn't clear.

I wasn't saying that you should incorporate those test cases onto this PR.

that wouldn't make sense anyway, since this PR is only a small step towards the goal.

I was just trying to make sure it was clear that the overall plan would need some kind of test from somewhere, especially since the test from #35637 wasn't exactly checking a big picture end-to-end behavior (it was more of a white-box test).