Skip to content

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) #136985

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Feb 21, 2025

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Feb 13, 2025

Accepted MCP: rust-lang/compiler-team#832

Fixes #135802

Do not consider the inhabitedness of a type for function call ABI purposes.

  • Remove the rustc_abi::BackendRepr::Uninhabited variant
    • Instead calculate the BackendRepr of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
  • Add an uninhabited: bool field to rustc_abi::LayoutData so inhabitedness of a LayoutData can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc @RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2025

r? @wesleywiser

rustbot has assigned @wesleywiser.
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 13, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2025

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@RalfJung
Copy link
Member

Cc @workingjubilee

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires a codegen test demonstrating the change.

@zachs18 zachs18 force-pushed the backend-repr-remove-uninhabited branch from 11153ef to bdc321b Compare February 14, 2025 22:38
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@zachs18
Copy link
Contributor Author

zachs18 commented Feb 14, 2025

bdc321b fixes codegen (I think) and adds a codegen test. (Previously codegen_call_terminator and make_return_dest assumed that a call with no target block always had a PassMode::Ignore'd return type, so wouldn't prepend the return-place pointer to llargs. Now, make_return_dest doesn't consider the target block, and codegen_call_terminator calls make_return_dest regardless of if there is a target block).

@workingjubilee workingjubilee self-assigned this Feb 17, 2025
@workingjubilee
Copy link
Member

cool! sounds like this is finding even more exciting issues in our codegen logic

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment nit, I kind of wish we didn't use another public field here but LayoutData has a lot of construction sites and that would become a much bigger refactor, sooo... whatever.

r=me with nit

@workingjubilee
Copy link
Member

@bors delegate+

@bors
Copy link
Collaborator

bors commented Feb 17, 2025

✌️ @zachs18, you can now approve this pull request!

If @workingjubilee told you to "r=me" after making some further change, please make that change, then do @bors r=@workingjubilee

@zachs18
Copy link
Contributor Author

zachs18 commented Feb 18, 2025

@bors r=@workingjubilee

@bors
Copy link
Collaborator

bors commented Feb 18, 2025

📌 Commit 85fe2b1 has been approved by workingjubilee

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. labels Feb 18, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Feb 18, 2025
…bited, r=workingjubilee

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited)

Accepted MCP: rust-lang/compiler-team#832

Fixes rust-lang#135802

Do not consider the inhabitedness of a type for function call ABI purposes.

* Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant
  * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
* Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang#135767 (Future incompatibility warning `unsupported_fn_ptr_calling_conventions`: Also warn in dependencies)
 - rust-lang#136457 (Expose algebraic floating point intrinsics)
 - rust-lang#136985 (Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited))
 - rust-lang#137000 (Deeply normalize item bounds in new solver)
 - rust-lang#137151 (Install more signal stack trace handlers)
 - rust-lang#137155 (Organize `OsString`/`OsStr` shims)
 - rust-lang#137161 (Pattern Migration 2024: fix incorrect messages/suggestions when errors arise in macro expansions)
 - rust-lang#137162 (Move methods from `Map` to `TyCtxt`, part 2.)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2025
…bited, r=workingjubilee

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited)

Accepted MCP: rust-lang/compiler-team#832

Fixes rust-lang#135802

Do not consider the inhabitedness of a type for function call ABI purposes.

* Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant
  * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
* Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc ``@RalfJung``
@@ -1274,11 +1274,11 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValueVisitor<'tcx, M> for ValidityVisitor<'rt,
// FIXME: We could avoid some redundant checks here. For newtypes wrapping
// scalars, we do the same check on every "level" (e.g., first we check
// MyNewtype and then the scalar in there).
if val.layout.is_uninhabited() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is now in a section titled "check the ABI", which doesn't make a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit addressing this.

@RalfJung
Copy link
Member

@bors r=@workingjubilee

@bors
Copy link
Collaborator

bors commented Feb 18, 2025

📌 Commit 8382b99 has been approved by workingjubilee

It is now in the queue for this repository.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2025
…bited, r=workingjubilee

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited)

Accepted MCP: rust-lang/compiler-team#832

Fixes rust-lang#135802

Do not consider the inhabitedness of a type for function call ABI purposes.

* Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant
  * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
* Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc `@RalfJung`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2025
…bited, r=workingjubilee

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited)

Accepted MCP: rust-lang/compiler-team#832

Fixes rust-lang#135802

Do not consider the inhabitedness of a type for function call ABI purposes.

* Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant
  * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
* Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc ``@RalfJung``
@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. labels Feb 20, 2025
@bors
Copy link
Collaborator

bors commented Feb 20, 2025

☔ The latest upstream changes (presumably #137058) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 20, 2025
…l` field in `LayoutData`.

Also update comments that refered to BackendRepr::Uninhabited.
@zachs18 zachs18 force-pushed the backend-repr-remove-uninhabited branch from ed34181 to 3020eef Compare February 20, 2025 19:34
zachs18 and others added 5 commits February 20, 2025 13:40
…turn ABI as wrapped type.

Fix codegen of uninhabited PassMode::Indirect return types.

Add codegen test for uninhabited PassMode::Indirect return types.

Enable optimizations for uninhabited return type codegen test
Co-authored-by: Jubilee <workingjubilee@gmail.com>
@zachs18 zachs18 force-pushed the backend-repr-remove-uninhabited branch from 3020eef to e3f5db0 Compare February 20, 2025 19:41
@workingjubilee
Copy link
Member

Cool!

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 20, 2025

📌 Commit e3f5db0 has been approved by workingjubilee

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 20, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 20, 2025
…bited, r=workingjubilee

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited)

Accepted MCP: rust-lang/compiler-team#832

Fixes rust-lang#135802

Do not consider the inhabitedness of a type for function call ABI purposes.

* Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant
  * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
* Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2025
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#131651 (Create a generic AVR target: avr-none)
 - rust-lang#136473 (infer linker flavor by linker name if it's sufficiently specific)
 - rust-lang#136608 (Pass through of target features to llvm-bitcode-linker and handling them)
 - rust-lang#136985 (Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited))
 - rust-lang#137192 (Remove obsolete Windows ThinLTO+TLS workaround)
 - rust-lang#137204 (Clarify MIR dialects and phases)
 - rust-lang#137270 (Fix `*-win7-windows-msvc` target since 26eeac1)
 - rust-lang#137298 (Check signature WF when lowering MIR body)
 - rust-lang#137299 (Simplify `Postorder` customization.)
 - rust-lang#137312 (Update references to cc_detect.rs)
 - rust-lang#137313 (Some codegen_llvm cleanups)
 - rust-lang#137318 (Workaround Cranelift not yet properly supporting vectors smaller than 128bit)
 - rust-lang#137322 (Update docs for default features of wasm targets)
 - rust-lang#137324 (Make x86 QNX target name consistent with other Rust targets)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#131651 (Create a generic AVR target: avr-none)
 - rust-lang#134340 (Stabilize `num_midpoint_signed` feature)
 - rust-lang#136473 (infer linker flavor by linker name if it's sufficiently specific)
 - rust-lang#136608 (Pass through of target features to llvm-bitcode-linker and handling them)
 - rust-lang#136985 (Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited))
 - rust-lang#137270 (Fix `*-win7-windows-msvc` target since 26eeac1)
 - rust-lang#137312 (Update references to cc_detect.rs)
 - rust-lang#137318 (Workaround Cranelift not yet properly supporting vectors smaller than 128bit)
 - rust-lang#137322 (Update docs for default features of wasm targets)
 - rust-lang#137324 (Make x86 QNX target name consistent with other Rust targets)
 - rust-lang#137338 (skip submodule updating logics on tarballs)
 - rust-lang#137340 (Add a notice about missing GCC sources into source tarballs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8c9e374 into rust-lang:master Feb 21, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 21, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2025
Rollup merge of rust-lang#136985 - zachs18:backend-repr-remove-uninhabited, r=workingjubilee

Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited)

Accepted MCP: rust-lang/compiler-team#832

Fixes rust-lang#135802

Do not consider the inhabitedness of a type for function call ABI purposes.

* Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant
  * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc)
* Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it).

This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types.

cc ``@RalfJung``
remi-delmas-3000 pushed a commit to remi-delmas-3000/kani that referenced this pull request Feb 26, 2025
Changes to attribute parsing and representation
rust-lang/rust#135726

Map methods moved to `TyCtx`
rust-lang/rust#137162
rust-lang/rust#137397

Remove `BackendRepr::Unihabited`
rust-lang/rust#136985
remi-delmas-3000 pushed a commit to remi-delmas-3000/kani that referenced this pull request Feb 26, 2025
Changes to attribute parsing and representation
rust-lang/rust#135726

Map methods moved to `TyCtx`
rust-lang/rust#137162
rust-lang/rust#137397

Remove `BackendRepr::Unihabited`
rust-lang/rust#136985
remi-delmas-3000 pushed a commit to remi-delmas-3000/kani that referenced this pull request Feb 27, 2025
Changes to attribute parsing and representation
rust-lang/rust#135726

Map methods moved to `TyCtx`
rust-lang/rust#137162
rust-lang/rust#137397

Remove `BackendRepr::Unihabited`
rust-lang/rust#136985
github-merge-queue bot pushed a commit to model-checking/kani that referenced this pull request Mar 5, 2025
Changes to attribute parsing and representation
rust-lang/rust#135726

Map methods moved to `TyCtx`
rust-lang/rust#137162
rust-lang/rust#137397

Remove `BackendRepr::Unihabited`
rust-lang/rust#136985

Intrinsics rint, roundeven, nearbyint replaced by `round_ties_even`.
 rust-lang/rust#136543.
Use `__sort_of_CPROVER_round_to_integral` to model `round_ties_even`.

Rename `sub_ptr` to `offset_from_unsigned`.
The feature gate is still `ptr_sub_ptr`.
rust-lang/rust#137483.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.

---------

Co-authored-by: Remi Delmas <delmasrd@amazon.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repr(transparent) wrapper with uninhabited ZST has different return ABI than wrapped field.
8 participants