Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ABI compatibility: remove section on target features #132136

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

RalfJung
Copy link
Member

Once #127731 lands, we will properly diagnose ABI issues caused by target feature mismatch (at least on tier 1 targets). So I'd say we can remove the corresponding part of the docs here -- this is now something the compiler can take care of, so programmers don't need to be concerned. For now this is just a lint, but that's just a transition period, like in prior cases where we fix I-unsound bugs by adding a new check that goes through the "future incompatibility" stages. We have decided that it's actually a bug that we have ABI risks around target features, and we shouldn't document that bug as-if it was intended behavior.

Cc @rust-lang/opsem @chorman0773 @veluca93

@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 25, 2024
@Mark-Simulacrum
Copy link
Member

I guess this is now waiting on #132173 due to the revert?

I'm wondering if we should keep this section, while noting that the requirements are typically guaranteed by the compiler. It's confusing to me to say this is just guaranteed by the compiler -- maybe I'm missing something, but we probably can't do that across e.g. language boundary / translation unit, where I manually write (or use bindgen, etc.) extern { ... } blocks to reference foreign functions? Do we have some other docs that specify how "closely" I need to match that foreign function's definition? Seems like these would be the docs I need to read...

@Mark-Simulacrum
Copy link
Member

The way it is fixed is by making it a hard error to call or declare an extern "C" function that passes a SIMD type by-value without enabling the target feature that enables SIMD vectors of the appropriate size. The reason it is made a hard error is that these functions end up having a different ABI depending on whether that target feature is enabled or not.

At least if this is the fix, it seems like while Rust may require you to enable the feature, it's not clear from that that GCC or Clang have similar enforcement, so I can still run into trouble?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 26, 2024 via email

@RalfJung
Copy link
Member Author

RalfJung commented Oct 26, 2024 via email

@Mark-Simulacrum
Copy link
Member

Ah, right. I remember that discussion when we added this :) In that case, r=me once dependency lands.

I do think it would be good to add an explicit note about FFI somewhere -- the page doesn't seem to mention that at all today in the actual text, only by omission -- and omission feels somewhat unfortunate :)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 26, 2024 via email

@Mark-Simulacrum
Copy link
Member

Well it talks about which Rust types are compatible with which Rust types. So it seems fairly clear that this cannot talk about C to/from Rust calls?

I'm not sure I agree. Without an explicit note, and with types like std::ffi::c_int being documented as "Equivalent to C’s signed int (int) type.", it seems plausible (expected!) for a user to interpret that as meaning a int typed argument on the C side is the same as c_int in Rust for an extern "C" function.

If we want to avoid such an assumption, I think adding "These guarantees/requirements only apply to Rust <> Rust calls. The requirements for cross language calls are not currently documented here." could help.

In any case, I think that's a separate topic -- let me file a PR proposing such language and we can discuss there.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2024
…g, r=RalfJung

Make clearer that guarantees in ABI compatibility are for Rust only

cc rust-lang#132136 (comment) -- it looks like we already had a note that I missed in my initial look here, but this goes further to emphasize the guarantees, including uplifting it to the top of the general documentation.

r? `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 28, 2024
…g, r=RalfJung

Make clearer that guarantees in ABI compatibility are for Rust only

cc rust-lang#132136 (comment) -- it looks like we already had a note that I missed in my initial look here, but this goes further to emphasize the guarantees, including uplifting it to the top of the general documentation.

r? `@RalfJung`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 30, 2024
…Jung

Make clearer that guarantees in ABI compatibility are for Rust only

cc rust-lang/rust#132136 (comment) -- it looks like we already had a note that I missed in my initial look here, but this goes further to emphasize the guarantees, including uplifting it to the top of the general documentation.

r? `@RalfJung`
@RalfJung RalfJung added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 7, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Nov 7, 2024

@Mark-Simulacrum so modulo waiting for #132173, is this good to go?

@Mark-Simulacrum
Copy link
Member

Yeah, r=me still stands when ready.

@RalfJung
Copy link
Member Author

Okay, great. :)

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 10, 2024

📌 Commit d199a63 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 10, 2024
@RalfJung
Copy link
Member Author

@bors rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2024
…iaskrgr

Rollup of 2 pull requests

Successful merges:

 - rust-lang#132136 (ABI compatibility: remove section on target features)
 - rust-lang#132816 (Dont suggest `use<impl Trait>` when we have an edition-2024-related borrowck issue)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 94cc01a into rust-lang:master Nov 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2024
Rollup merge of rust-lang#132136 - RalfJung:target-feature-abi-compat, r=Mark-Simulacrum

ABI compatibility: remove section on target features

Once rust-lang#127731 lands, we will properly diagnose ABI issues caused by target feature mismatch (at least on tier 1 targets). So I'd say we can remove the corresponding part of the docs here -- this is now something the compiler can take care of, so programmers don't need to be concerned. For now this is just a lint, but that's just a transition period, like in prior cases where we fix I-unsound bugs by adding a new check that goes through the "future incompatibility" stages. We have decided that it's actually a bug that we have ABI risks around target features, and we shouldn't document that bug as-if it was intended behavior.

Cc `@rust-lang/opsem` `@chorman0773` `@veluca93`
@RalfJung RalfJung deleted the target-feature-abi-compat branch November 10, 2024 18:08
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…, r=Mark-Simulacrum

ABI compatibility: remove section on target features

Once rust-lang#127731 lands, we will properly diagnose ABI issues caused by target feature mismatch (at least on tier 1 targets). So I'd say we can remove the corresponding part of the docs here -- this is now something the compiler can take care of, so programmers don't need to be concerned. For now this is just a lint, but that's just a transition period, like in prior cases where we fix I-unsound bugs by adding a new check that goes through the "future incompatibility" stages. We have decided that it's actually a bug that we have ABI risks around target features, and we shouldn't document that bug as-if it was intended behavior.

Cc `@rust-lang/opsem` `@chorman0773` `@veluca93`
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…iaskrgr

Rollup of 2 pull requests

Successful merges:

 - rust-lang#132136 (ABI compatibility: remove section on target features)
 - rust-lang#132816 (Dont suggest `use<impl Trait>` when we have an edition-2024-related borrowck issue)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants