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 compat check: detect unadjusted ABI mismatches #129649

Merged
merged 1 commit into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion compiler/rustc_abi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1700,7 +1700,9 @@ impl<FieldIdx: Idx, VariantIdx: Idx> LayoutS<FieldIdx, VariantIdx> {

/// Checks if these two `Layout` are equal enough to be considered "the same for all function
/// call ABIs". Note however that real ABIs depend on more details that are not reflected in the
/// `Layout`; the `PassMode` need to be compared as well.
/// `Layout`; the `PassMode` need to be compared as well. Also note that we assume
/// aggregates are passed via `PassMode::Indirect` or `PassMode::Cast`; more strict
/// checks would otherwise be required.
pub fn eq_abi(&self, other: &Self) -> bool {
// The one thing that we are not capturing here is that for unsized types, the metadata must
// also have the same ABI, and moreover that the same metadata leads to the same size. The
Expand Down
19 changes: 17 additions & 2 deletions compiler/rustc_target/src/abi/call/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -745,10 +745,25 @@ impl<'a, Ty> ArgAbi<'a, Ty> {

/// Checks if these two `ArgAbi` are equal enough to be considered "the same for all
/// function call ABIs".
pub fn eq_abi(&self, other: &Self) -> bool {
pub fn eq_abi(&self, other: &Self) -> bool
where
Ty: PartialEq,
{
// Ideally we'd just compare the `mode`, but that is not enough -- for some modes LLVM will look
// at the type.
self.layout.eq_abi(&other.layout) && self.mode.eq_abi(&other.mode)
self.layout.eq_abi(&other.layout) && self.mode.eq_abi(&other.mode) && {
// `fn_arg_sanity_check` accepts `PassMode::Direct` for some aggregates.
// That elevates any type difference to an ABI difference since we just use the
// full Rust type as the LLVM argument/return type.
if matches!(self.mode, PassMode::Direct(..))
&& matches!(self.layout.abi, Abi::Aggregate { .. })
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these PassMode::Direct concerns irrelevant for non-aggregates?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes -- for non-aggregates, codegen uses the layout.abi information to compute how the argument should be passed, which has already been checked to be sufficiently compatible. It is only with Aggregate that the underlying Rust type "leaks through".

{
// For aggregates in `Direct` mode to be compatible, the types need to be equal.
self.layout.ty == other.layout.ty
} else {
true
}
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_target/src/spec/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ pub enum Abi {
},
RustIntrinsic,
RustCall,
/// *Not* a stable ABI, just directly use the Rust types to describe the ABI for LLVM. Even
/// normally ABI-compatible Rust types can become ABI-incompatible with this ABI!
Unadjusted,
/// For things unlikely to be called, where reducing register pressure in
/// `extern "Rust"` callers is worth paying extra cost in the callee.
Expand Down
7 changes: 4 additions & 3 deletions tests/ui/abi/compatibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,10 @@
//@[loongarch64] compile-flags: --target loongarch64-unknown-linux-gnu
//@[loongarch64] needs-llvm-components: loongarch
//@[loongarch64] min-llvm-version: 18
//@ revisions: wasm
//@[wasm] compile-flags: --target wasm32-unknown-unknown
//@[wasm] needs-llvm-components: webassembly
//FIXME: wasm is disabled due to <https://github.com/rust-lang/rust/issues/115666>.
//FIXME @ revisions: wasm
//FIXME @[wasm] compile-flags: --target wasm32-unknown-unknown
//FIXME @[wasm] needs-llvm-components: webassembly
//@ revisions: wasip1
//@[wasip1] compile-flags: --target wasm32-wasip1
//@[wasip1] needs-llvm-components: webassembly
Expand Down
Loading