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

Remove i128 and u128 from improper_ctypes_definitions #137306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Feb 20, 2025

Rust's 128-bit integers have historically been incompatible with C 1. However, there have been a number of changes in Rust and LLVM that mean this is no longer the case:

At rust-lang/lang-team#255 (comment), the lang team considered it acceptable to remove i128 from improper_ctypes_definitions if the LLVM version is known to be compatible. Time has elapsed since then and we have dropped support for LLVM versions that do not have the x86 fixes, meaning a per-llvm-version lint should no longer be necessary. The PowerPC, SPARC, and MIPS changes only came in LLVM 20 but since Rust's datalayouts have also been updated to match, we will be using the correct alignment regardless of LLVM version.

Closes: #134288

Rust's 128-bit integers have historically been incompatible with C [1].
However, there have been a number of changes in Rust and LLVM that
mean this is no longer the case:

* Incorrect alignment of `i128` on x86 [1]: adjusting Rust's alignment
  proposed at rust-lang/compiler-team#683,
  implemented at rust-lang#116672.
* LLVM version of the above: resolved in LLVM, including ABI fix.
  Present in LLVM18 (our minimum supported version).
* Incorrect alignment of `i128` on 64-bit PowerPC, SPARC, and MIPS [2]:
  Rust's data layouts adjusted at
  rust-lang#132422,
  rust-lang#132741,
  rust-lang#134115.
* LLVM version of the above: done in LLVM 20
  llvm/llvm-project#102783.
* Incorrect return convention of `i128` on Windows: adjusted to match
  GCC and Clang at rust-lang#134290.

At [3], the lang team considered it acceptable to remove `i128` from
`improper_ctypes_definitions` if the LLVM version is known to be
compatible. Time has elapsed since then and we have dropped support for
LLVM versions that do not have the x86 fixes, meaning a per-llvm-version
lint should no longer be necessary. The PowerPC, SPARC, and MIPS changes
only came in LLVM 20 but since Rust's datalayouts have also been updated
to match, we will be using the correct alignment regardless of LLVM
version.

Closes: rust-lang#134288
Closes: rust-lang#128950

[1]: rust-lang#54341
[2]: rust-lang#128950
[3]: rust-lang/lang-team#255 (comment)
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
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 20, 2025
@tgross35
Copy link
Contributor Author

I am not aware of any other incompatibilities at this time, @beetrees is there anything outstanding?

I believe this needs a lang FCP @rust-lang/lang.

@rustbot label +T-lang, +I-lang-nominated, +I-lang-easy-decision

@rustbot rustbot added I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 20, 2025
@RalfJung
Copy link
Member

Cc @workingjubilee

@programmerjake
Copy link
Member

I may be misunderstanding, but it seems like rustc is removing the fixes from the data layout string when it detects LLVM < 20, so unless that's changed to not remove the 128-bit layout from the layout string, rustc would still be broken on LLVM < 20:

if llvm_version < (20, 0, 0) {
if sess.target.arch == "aarch64" || sess.target.arch.starts_with("arm64") {
// LLVM 20 defines three additional address spaces for alternate
// pointer kinds used in Windows.
// See https://github.com/llvm/llvm-project/pull/111879
target_data_layout =
target_data_layout.replace("-p270:32:32-p271:32:32-p272:64:64", "");
}
if sess.target.arch.starts_with("sparc") {
// LLVM 20 updates the sparc layout to correctly align 128 bit integers to 128 bit.
// See https://github.com/llvm/llvm-project/pull/106951
target_data_layout = target_data_layout.replace("-i128:128", "");
}
if sess.target.arch.starts_with("mips64") {
// LLVM 20 updates the mips64 layout to correctly align 128 bit integers to 128 bit.
// See https://github.com/llvm/llvm-project/pull/112084
target_data_layout = target_data_layout.replace("-i128:128", "");
}
if sess.target.arch.starts_with("powerpc64") {
// LLVM 20 updates the powerpc64 layout to correctly align 128 bit integers to 128 bit.
// See https://github.com/llvm/llvm-project/pull/118004
target_data_layout = target_data_layout.replace("-i128:128", "");
}

@beetrees
Copy link
Contributor

All the sizes and alignments of i128 match __int128 on 64-bit targets; I haven't checked the ABIs in depth but I'm not aware of any incompatibilities with __int128 (it would probably be good to use something like abi-cafe to check the ABIs). _BitInt(128) currently has a different alignment than __int128 on x86-64: it would be good to document somewhere which one Rust's i128 is compatible with.

While 32-bit platforms don't support __int128, they do support _BitInt(128), so if the lint is going to be removed on all platforms (not just 64-bit platforms) then I'd expect i128 and _BitInt(128) to be ABI compatible on 32-bit platforms. Spot checking a few targets now, I've discovered:

  • On x86-32, Clang and GCC give _BitInt(128) an alignment of 4 whereas Rust gives i128 an alignment of 16. Additionally, Rust and GCC pass i128/_BitInt(128) directly, whereas Clang passes _BitInt(128) indirectly.
  • On 32-bit PowerPC, Rust passes i128 directly in registers whereas Clang passes _BitInt(128) indirectly.

Additionally, the Clang documentation states:

Note: the ABI for _BitInt(N) is still in the process of being stabilized, so this type should not yet be used in interfaces that require ABI stability.

@programmerjake
Copy link
Member

programmerjake commented Feb 20, 2025

_BitInt(128) currently has a different alignment than __int128 on x86-64: it would be good to document somewhere which one Rust's i128 is compatible with.

ideally x86-64 would fix its broken ABIs so __int128 and _BitInt(128) have the same ABI.

see https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11

@beetrees
Copy link
Contributor

I may be misunderstanding, but it seems like rustc is removing the fixes from the data layout string when it detects LLVM < 20, so unless that's changed to not remove the 128-bit layout from the layout string, rustc would still be broken on LLVM < 20:

The alignment is only removed when passing the layout to LLVM, so this will only matter if LLVM uses the alignment itself for e.g. low-level function call ABI details. I'm not aware of any cases that this happens for the affected targets, but I haven't looked into it in depth so it's possible there are cases where it matters.

@bjorn3
Copy link
Member

bjorn3 commented Feb 20, 2025

I would expect i128 arguments that are passed on the stack to be aligned to 16 bytes with the fix and to be aligned to 4 or 8 bytes without the fix. This changes the offset where the argument is passed and thus affects the ABI.

@nikic
Copy link
Contributor

nikic commented Feb 20, 2025

While 32-bit platforms don't support __int128, they do support _BitInt(128), so if the lint is going to be removed on all platforms (not just 64-bit platforms) then I'd expect i128 and _BitInt(128) to be ABI compatible on 32-bit platforms. Spot checking a few targets now, I've discovered:

Given the fact that i128 maps to __int128, but not _BitInt(128) on x86_64, I don't think that mapping it to _BitInt(128) on other platforms is a good idea.

I think it would be preferable to treat i128 as not a proper FFI type on platforms that do not actually specify an ABI for __int128.

@petrochenkov
Copy link
Contributor

r? @workingjubilee

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@petrochenkov
Copy link
Contributor

r? @RalfJung

@rustbot rustbot assigned RalfJung and unassigned petrochenkov Feb 20, 2025
@RalfJung
Copy link
Member

RalfJung commented Feb 20, 2025 via email

@rustbot rustbot assigned nikic and unassigned RalfJung Feb 20, 2025
@workingjubilee workingjubilee removed the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Feb 20, 2025
@workingjubilee
Copy link
Member

I don't actually think this will be an easy decision.

@beetrees
Copy link
Contributor

I would expect i128 arguments that are passed on the stack to be aligned to 16 bytes with the fix and to be aligned to 4 or 8 bytes without the fix. This changes the offset where the argument is passed and thus affects the ABI.

I've just checked; 64-bit PowerPC, 64-bit MIPS and 64-bit SPARC all appear to only align i128 to 8 bytes when it is passed as an argument on the stack, even with LLVM 20 (this is consistent with the PowerPC64 ELFv2 ABI specification).

@workingjubilee
Copy link
Member

This FIXME does not seem to be addressed for 64-bit Power:

// FIXME:
// Alignment of 128 bit types is not currently handled, this will
// need to be fixed when PowerPC vector support is added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. 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. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust's i128 return ABI does not agree with Clang and GCC on Windows targets
9 participants