remove pref_align_of intrinsic #90877
Conversation
|
Some changes occured to rustc_codegen_cranelift cc @bjorn3 Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
|
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
no_pref_align_of
branch
from
9ec424f to
3d75550
Compare
|
The Zulip thread does mention that this is used by c2rust though. |
|
Good point, I pinged the person who wrote that on Zulip (not sure about their GH handle). AFAIK we usually do remove features that have no path to stabilization. This one does not even have a tracking issue (to my knowledge) -- the least we should have if anyone actually needs this. |
|
However, https://github.com/immunant/c2rust/search?q=pref_align_to turns up no results. |
|
Hm, so it seems like the |
oops
By that argument, we should aim to have a function in |
|
Interesting, back in 2014, |
|
Ah yeah, I meant warranted in a sense that “this will produce the expected result” and less so a justification for use of an intrinsic as an implementation strategy. None of the mechanisms to obtain the preferred alignment in C are standard either, though, so c2rust could consider not supporting preferred alignment too. |
|
I assume there will be pre-C11 code that uses |
|
Since this concerns intrinsics, Cc @rust-lang/lang |
|
With my lang hat on, I consider intrinsics entirely a compiler-team question so long as they're not otherwise exposed. So removing something unexposed sounds fine to me, FWIW. (I have no opinion right now on whether it should be exposed.) |
Well, they are unstably 'exposed' and the main concern here is breaking c2rust which uses one of those intrinsics that are not really but de-facto sufficiently exposed. |
apiraino
added
the
T-compiler
|
Even though I'm not really familiar with this intrinsic, my thoughts are: 1) What is/was its original purpose? 2) Is that purpose no longer needed or is it solved some other way? 3) If it isn't solved some other way, should it be? 4) How often is this actually used/how much breakage do we expect? |
|
@RalfJung what do you think the next step here is? I don't feel comfortable approving this alone (at least not without the questions above answered, maybe also with lang FCP given this is unstably exposed). |
|
Looks like some team needs to decide if we want to keep an unexposed un-RFCd no-tracking-issue intrinsic around because it has a downstream user. I don't think this is a good situation. If we didn't already have it, we certainly would not add it like that. Ideally, someone who needs that intrinsic would work towards exposing it, unstably at least. |
scottmcm
added
the
I-lang-nominated
|
It definitely doesn't need an FCP, since it's a reversible decision that doesn't affect stable. I've nominated for lang to try to get unstuck. |
|
Cc @thedataking for c2rust input. |
|
Appreciate the ping @RalfJung. The code which may emit this intrinsic was added to support LLVM 8 (commit). The LLVM source code for
Which sounds like we'd lose the ability to correctly transpile some code if preferred alignment is no longer exposed; we would have to emit an error if the input source uses this intrinsic. |
|
Here are the relevant GCC bug reports about this:
I'm not sure exactly the sense in which the preferred alignment is ABI impacting. It will of course affect the ABI of things like Emitting an error on |
jackh726
added
S-waiting-on-team
|
We discussed this both in last week's and today's @rust-lang/lang meeting. In both cases, we came to the conclusion that we're not in a hurry to remove this intrinsic, and it's not obvious that the intrinsic couldn't be stabilized in the future if it makes sense to do so, given that people are using it. Given that, we don't want to merge this PR at this time, and we don't mind letting this sit on nightly, unless there's some other reason that we should be considering this an urgent issue. |
|
Okay, I opened a tracking issue at #91971 then. |
…lacrum link to pref_align_of tracking issue If we are not going to remove this intrinsic (rust-lang#90877), I think we should at least have a place to centralize discussion around it, so here we go. Intrinsics don't have their own separate features and usually we instead use the public method for tracking it, but this one does not have such a method... so the tracking issue is just a regular link. (And then we sue it for the const part as well.)
…lacrum link to pref_align_of tracking issue If we are not going to remove this intrinsic (rust-lang#90877), I think we should at least have a place to centralize discussion around it, so here we go. Intrinsics don't have their own separate features and usually we instead use the public method for tracking it, but this one does not have such a method... so the tracking issue is just a regular link. (And then we sue it for the const part as well.)
…lacrum link to pref_align_of tracking issue If we are not going to remove this intrinsic (rust-lang#90877), I think we should at least have a place to centralize discussion around it, so here we go. Intrinsics don't have their own separate features and usually we instead use the public method for tracking it, but this one does not have such a method... so the tracking issue is just a regular link. (And then we sue it for the const part as well.)
…lacrum link to pref_align_of tracking issue If we are not going to remove this intrinsic (rust-lang#90877), I think we should at least have a place to centralize discussion around it, so here we go. Intrinsics don't have their own separate features and usually we instead use the public method for tracking it, but this one does not have such a method... so the tracking issue is just a regular link. (And then we sue it for the const part as well.)
In rust-lang#90877 (comment) T-lang decided they did not wish to remove `intrinsics::pref_align_of`. However, the intrinsic 1. is a nightly feature, so can be removed at compiler/libs discretion 2. requires considerable effort in the compiler to support 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[^1], in ways Rust would not interact with, even with increased C++ interop 4. the AIX XLC compiler's layout rules for C have unrelated concerns[^2] 5. otherwise it is used to pad static layout[^3], not correctness 6. these static align-up clauses can be replaced by other rules 7. there is only one clear benefactor we are aware of: tools automating C -> Rust translation handling GNU extensions[^4]. 8. because the GNU extension is a "false friend" of a standard C form, `alignof` or `_Alignof`, the C code likely meant `mem::align_of`, which makes the choice to support such a mapping very questionable 9. its presence makes it easy to do incorrect codegen with the compiler, as Rust rules regarding alignment (e.g. size == align * N) do not hold with preferred alignment, and this actually happened[^5] Thus remove the intrinsic and supporting code, as a cost-benefit analysis cannot possibly justify going to such lengths and risking correctness just to help automated translation of an ill-considered GNU extension. [^1]: cite AIX cookie layout [^2]: cite AIX "power alignment" issues [^3]: cite static alignment usages [^4]: cite c2rust commit [^5]: cite bjorn3's PR
In rust-lang#90877 (comment) T-lang decided they did not wish to remove `intrinsics::pref_align_of`. However, the intrinsic 1. is a nightly feature, so can be removed at compiler/libs discretion 2. requires considerable effort in the compiler to support 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[^1], in ways Rust would not interact with, even with increased C++ interop 4. the AIX XLC compiler's layout rules for C have unrelated concerns[^2] 5. otherwise it is used to pad static layout[^3], not correctness 6. these static align-up clauses can be replaced by other rules 7. there is only one clear benefactor we are aware of: tools automating C -> Rust translation handling GNU extensions[^4]. 8. because the GNU extension is a "false friend" of a standard C form, `alignof` or `_Alignof`, the C code likely meant `mem::align_of`, which makes the choice to support such a mapping very questionable 9. its presence makes it easy to do incorrect codegen with the compiler, as Rust rules regarding alignment (e.g. size == align * N) do not hold with preferred alignment, and this actually happened[^5] Thus remove the intrinsic and supporting code, as a cost-benefit analysis cannot possibly justify going to such lengths and risking correctness just to help automated translation of an ill-considered GNU extension. [^1]: cite AIX cookie layout [^2]: cite AIX "power alignment" issues [^3]: cite static alignment usages [^4]: cite c2rust commit [^5]: cite bjorn3's PR
In rust-lang#90877 (comment) T-lang decided they did not wish 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 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[^1], in ways Rust would not consider even with increased C++ interop 4. is dwarfed by unrelated concerns regarding the AIX XLC compiler's layout rules for C which make it difficult to support[^2] 5. is only used by rustc to pad global layouts[^3], not correctness 6. can be replaced by other rules concerning globals 7. has only one clear benefactor: automating C -> Rust translation for GNU extensions[^4] like `__alignof`. 8. was likely intended to be `alignof`, `_Alignof`, AKA `mem::align_of`, because the GNU extensions were "false friends" of Standard C forms, which makes the choice to support such a mapping very questionable 9. makes it easy to do incorrect codegen with the compiler by its mere presence, as usual Rust rules of alignment (e.g. size == align * N) do not hold with preferred alignment aand this actually happened[^5] Thus remove the intrinsic and supporting code, as a cost-benefit analysis cannot possibly justify going to such lengths and risking correctness just to help automated translation of an ill-considered GNU extension. [^1]: cite AIX cookie layout [^2]: cite AIX "power alignment" issues [^3]: cite static alignment usages [^4]: cite c2rust commit [^5]: cite bjorn3's PR
In rust-lang#90877 (comment) T-lang decided they did not wish 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 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[^1], in ways Rust would not consider even with increased C++ interop 4. is dwarfed by unrelated concerns regarding the AIX XLC compiler's layout rules for C which make it difficult to support[^2] 5. is only used by rustc to pad global layouts[^3], not correctness 6. can be replaced by other rules concerning globals, as it only affects alignments for a few types under 16 bytes of alignment 7. has only one clear benefactor: automating C -> Rust translation for GNU extensions[^4] like `__alignof` 8. was likely intended to be `alignof`, `_Alignof`, AKA `mem::align_of`, because the GNU extensions were "false friends" of Standard C forms, which makes the choice to support such a mapping very questionable 9. makes it easy to do incorrect codegen with the compiler by its mere presence, as usual Rust rules of alignment (e.g. size == align * N) do not hold with preferred alignment[^5] Thus remove the intrinsic and supporting code, as a cost-benefit analysis cannot possibly justify going to such lengths and risking correctness just to help automated translation of an ill-considered GNU extension. [^1]: cite AIX cookie layout [^2]: cite AIX "power alignment" issues [^3]: cite static alignment usages [^4]: cite c2rust commit [^5]: cite bjorn3's PR
In rust-lang#90877 (comment) T-lang decided they did not wish 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[^1], in ways Rust would not consider even with increased C++ interop 4. is dwarfed by unrelated concerns regarding the AIX XLC compiler's layout rules for C which make it difficult to support[^2] 5. is only used by rustc to pad global layouts[^3], not correctness 6. can be adequately replaced by other rules for globals, as it mostly affects alignments for a few types under 16 bytes of alignment 7. has only one clear benefactor: automating C -> Rust translation for GNU extensions[^4] like `__alignof` 8. was likely intended to be `alignof`, `_Alignof`, AKA `mem::align_of`, because the GNU extensions were "false friends" of Standard C forms, which makes the choice to support such a mapping very questionable 9. makes it easy to do incorrect codegen with the compiler by its mere presence, as usual Rust rules of alignment (e.g. size == align * N) do not hold with preferred alignment[^5] Thus remove the intrinsic and supporting code, as a cost-benefit analysis cannot possibly justify going to such lengths and risking correctness just to help automated translation of an ill-considered GNU extension. [^1]: cite AIX cookie layout [^2]: cite AIX "power alignment" issues [^3]: cite static alignment usages [^4]: cite c2rust commit [^5]: cite bjorn3's PR
In rust-lang#90877 (comment) T-lang decided they did not wish 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[^1], in ways Rust would not consider even with increased C++ interop 4. is overshadowed by the AIX XLC compiler's "power alignment" rules which make even "C" compatibility difficult to support[^2] 5. is only used by rustc to pad global layouts[^3], not correctness 6. can be adequately replaced by other rules for globals, as it mostly affects alignments for a few types under 16 bytes of alignment 7. has only one clear benefactor: automating C -> Rust translation for GNU extensions[^4] like `__alignof` 8. was likely intended to be `alignof`, `_Alignof`, AKA `mem::align_of`, because the GNU extensions were "false friends" of Standard C forms, which makes the choice to support such a mapping very questionable 9. makes it easy to do incorrect codegen with the compiler by its mere presence, as usual Rust rules of alignment (e.g. size == align * N) do not hold with preferred alignment[^5] Thus remove the intrinsic and supporting code, as a cost-benefit analysis cannot possibly justify going to such lengths and risking correctness just to help automated translation of an ill-considered GNU extension. [^1]: cite AIX cookie layout [^2]: cite AIX "power alignment" issues [^3]: cite static alignment usages [^4]: cite c2rust commit [^5]: cite bjorn3's PR
This intrinsic has never been stably exposed, and its existence occasionally causes confusion. Preferred alignment still exists as a concept inside the compiler that it might use e.g. for aligning globals, but there seems to be no good reason to expose it to clients. Also see this discussion on Zulip.