Remove rustc's notion of "preferred" alignment AKA __alignof
#141803
Conversation
e3a7b51 to
72231a5
Compare
|
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
72231a5 to
a38ddd1
Compare
| @@ -3489,15 +3489,6 @@ pub const fn size_of<T>() -> usize; | ||
| #[rustc_intrinsic] | ||
| pub const fn min_align_of<T>() -> usize; |
There was a problem hiding this comment.
Also in the future, we should then rename this to just align_of.
|
I agree with removing the intrinsic. However, the PR description is written in a style that seems somewhat antagonistic to t-lang, which is wholly unnecessary. Quoting some t-lang members (not necessarily speaking for the team) from the prior PR:
It's not become an "urgent" issue, but the past 3 years have not shown any attempt of this moving any closer to stabilization -- and meanwhile we constantly pay the complexity cost in the compiler. An attempt at exposing this would have to include a clear idea of what "preferred alignment" even means, which is a discussion that would be good to have for a clean implementation. IMO that is enough to justify removal. However, we could help ease the transition by marking it as deprecated for a cycle, pointing to a tracking issue so that nightly users relying on it have a chance to speak up. |
|
@RalfJung To be clear, codegen backends have miscompiled Rust code by dint of this intrinsic relying on data that can undermine Rust assumptions: rust-lang/rustc_codegen_cranelift#1560 I am not sure "urgent" is the word but perhaps "should have been fixed yesterday"? |
"Should have been fixed some time in the last 10 years" :D (but waiting another few weeks to give people a heads-up is not a big deal) |
|
I... don't know if it is indeed not a big deal? I opened this because I remembered that I have to fix it before I do much more meaningful work on layout-related code. We've waited three years for someone to explain why they really need this and nothing has happened. |
|
I would rather you just close my PR than pretend we're actually helping anyone by implementing a deprecation cycle for a feature no one really wants. |
|
All I did was raise a concern that given how long this intrinsic was around, people might be relying on it, so it might be courteous to give them a heads-up. So far, we have not given a clear signal that this will disappear. OTOH it is guarded by an "internal" feature gate so whoever was using this should be aware they are on thin ice. If others don't think we need to be this careful, that's fine for me as well, but I think this option should at least be considered. |
I find this is sufficient signal to not need a deprecation cycle TBH. If the codegen reviewer agrees with removing this instrinsic, I'd say we should just rid of it especially if it's blocking further changes. |
|
The only uses on github are either declaring the intrinsics, copies of the rust repo or the sole usage inside c2rust itself for |
a38ddd1 to
613c6d6
Compare
|
I'm not particularly trying to give T-lang a super-hard time here, but they made a decision, and I don't think this PR makes sense without including a rebuttal of that decision. If anyone should get an earful, it's whoever working on GCC thought it was a good idea to retain |
|
☔ The latest upstream changes (presumably #142003) made this pull request unmergeable. Please resolve the merge conflicts. |
613c6d6 to
676b32b
Compare
This comment has been minimized.
This comment has been minimized.
676b32b to
6b4590d
Compare
In PR 90877 T-lang decided not to remove `intrinsics::pref_align_of`.
However, the intrinsic and its supporting code
1. is a nightly feature, so can be removed at compiler/libs discretion
2. requires considerable effort in the compiler to support, as it
necessarily complicates every single site reasoning about alignment
3. has been justified based on relevance to codegen, but it is only a
requirement for C++ (not C, not Rust) stack frame layout for AIX,
in ways Rust would not consider even with increased C++ interop
4. is only used by rustc to overalign some globals, not correctness
5. can be adequately replaced by other rules for globals, as it mostly
affects alignments for a few types under 16 bytes of alignment
6. has only one clear benefactor: automating C -> Rust translation
for GNU extensions like `__alignof`
7. such code was likely intended to be `alignof` or `_Alignof`,
because the GNU extension is a "false friend" of the C keyword,
which makes the choice to support such a mapping very questionable
8. makes it easy to do incorrect codegen in the compiler by its mere
presence as usual Rust rules of alignment (e.g. `size == align * N`)
do not hold with preferred alignment
The implementation is clearly damaging the code quality of the compiler.
Thus it is within the compiler team's purview to simply rip it out.
If T-lang wishes to have this intrinsic restored for c2rust's benefit,
it would have to use a radically different implementation that somehow
does not cause internal incorrectness.
Until then, remove the intrinsic and its supporting code, as one tool
and an ill-considered GCC extension cannot justify risking correctness.
Because we touch a fair amount of the compiler to change this at all,
and unfortunately the duplication of AbiAndPrefAlign is deep-rooted,
we keep an "AbiAlign" type which we can wean code off later.
6b4590d to
56c606c
Compare
We will want to remove many cases of `.abi`, including `.abi.thing`, so this may simplify future PRs and certainly doesn't hurt. We omit DerefMut because mutation is much rarer and localized.
|
rebased because conflict and also "x.py wonky" and also pushed another commit motivated by making the future transition smoother. |
In PR #90877 T-lang decided not to remove
intrinsics::pref_align_of. However, the intrinsic and its supporting code__alignof3alignofor_Alignof, because the GNU extension is a "false friend" of the C keyword, which makes the choice to support such a mapping very questionablesize == align * N) do not hold with preferred alignment4Despite an automated translation tool like c2rust using it, we have made multiple attempts to find a crate that actually has been committed to public repositories, like GitHub or crates.io, using such translated code. We have found none. While it is possible someone privately uses this intrinsic, it seems unlikely, and it is behind a feature gate that will warn about using the internal features of rustc.
The implementation is clearly damaging the code quality of the compiler. Thus it is within the compiler team's purview to simply rip it out. If T-lang wishes to have this intrinsic restored for c2rust's benefit, it would have to use a radically different implementation that somehow does not cause internal incorrectness.
Until then, remove the intrinsic and its supporting code, as one tool and an ill-considered GCC extension cannot justify risking correctness.
Because we touch a fair amount of the compiler to change this at all, and unfortunately the duplication of AbiAndPrefAlign is deep-rooted, we keep an "AbiAlign" type which we can wean code off later.
r? @bjorn3
Footnotes
https://github.com/rust-lang/rust/issues/91971#issuecomment-2451330704 ↩
as viewable in the code altered by this PR ↩
c2rust: https://github.com/immunant/c2rust/blame/3b1ec86b9b0cf363adfd3178cc45a891a970eef2/c2rust-transpile/src/translator/mod.rs#L3175 ↩
https://github.com/rust-lang/rustc_codegen_cranelift/issues/1560 ↩