Remove i128 and u128 from improper_ctypes_definitions by tgross35 · Pull Request #137306 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 Set alignment of i128 to 128 bits for x86 compiler-team#683, implemented at LLVM 18 x86 data layout update #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 llvm: Match new LLVM 128-bit integer alignment on sparc #132422, Update mips64 data layout to match LLVM 20 change #132741, rustc_target: ppc64 target string fixes for LLVM 20 #134115.
• LLVM version of the above: done in LLVM 20 LLVM data layout for i128 doesn't match Clang's alignment of __int128_t on 64-bit PowerPC, 64-bit SPARC and 64-bit MIPS llvm/llvm-project#102783.
• Incorrect return convention of i128 on Windows: adjusted to match GCC and Clang at Windows x86: Change i128 to return via the vector ABI #134290.

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.

Notes and unresolved questions from this thread:

• i128 should be layout- and ABI-compatible with __i128
• For platforms that provide _BitInt(128) but not __int128, we may not be compatible Remove i128 and u128 from improper_ctypes_definitions #137306 (comment). What should we do here?
  • Attempt to be compatible with _BitInt
  • Leave as-is and raise improper_ctypes_definitions on platforms without __int128 Remove i128 and u128 from improper_ctypes_definitions #137306 (comment)

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

[3]: rust-lang/lang-team#255 (comment)

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 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

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 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

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:

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.

_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

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.

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.

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.

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.

Sorry, I don't have the low level ABI / LLVM knowledge to review this. r? @nikic

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

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).

traviscross removed the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Mar 10, 2025

Let's kick off the FCP, then I have some asks to file as concerns.

@rfcbot fcp merge

cc @rust-lang/opsem @wesleywiser (who involved in one of the earlier threads)

@rfcbot concern update-per-nikic

Let's please update the stabilization report to address the questions raised in #137306 (comment) and discussed in this thread. If we're happy with the plan proposed by @nikic in #137306 (comment),

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.

then let's please update things to describe that.

What's the status of having an ABI expert group in some form? They would be the right ones to ping here.
Cc @chorman0773

@rfcbot concern documentation

It feels like we should be documenting this change persistently somewhere. Where's the right place for that?

cc @ehuss

What's the status of having an ABI expert group in some form? They would be the right ones to ping here.

Collecting nominations for members for it.

Also:

cc @rust-lang/wg-llvm @bjorn3

To affirm one of the above points, we probably shouldn't fall back to being compatible with _BitInt(128) if __int128 doesn't exist because if the platforms do add __int128 (or int128_t if the max_align_t issue is eventually resolved), that may not be compatible with _BitInt(128). I thought we may have unofficially come to the same conclusion at some point in the past, but I can't find anything about that now.

So +1 to Nikita's proposal, I can make that update if there is some level of consensus (looks like it).

One unfortunate downside is that most users of improper_ctypes_definitions will need to retain the allow if the crate is ever built on 32-bit targets, at least in cases where i128 is used for Rust-Rust calls as opposed to actually interfacing with C libraries.

Is there precedent for improper_ctypes being target-dependent? That seems problematic as the crate author seems likely to not see the diagnostic and users will not see it either when it occurs in a dependency.

It feels like we should be documenting this change persistently somewhere. Where's the right place for that?

AFAIK, we currently don't document the specifics of what is "improper". If we were to do that, I think it would likely be with the lint docs in improper-ctypes and improper-ctypes-definitions.

I suspect it is not documented because the number of things that is "improper" is large, it would be difficult to keep that documentation up-to-date, and the diagnostic is supposed to explain to you why it is improper (like "string slices have no C equivalent") so in most cases the user gets just-in-time documentation via the compiler output.

Makes sense.

@rfcbot resolve documentation

Is there precedent for improper_ctypes being target-dependent? That seems problematic as the crate author seems likely to not see the diagnostic and users will not see it either when it occurs in a dependency.

AFAIK no: the closest would be AIX having a lint for when Rust's repr(C) struct layout doesn't match the C compiler's, but that's done via a separate uses_power_alignment lint (added in #135552).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Apr 24, 2025

…li-obk

Add #[repr(u128)]/#[repr(i128)] enums to improper_ctypes_definitions

This makes them warn whenever a plain u128/i128 would. If the lang team decides to merge rust-lang#137306 then this can be reverted.

Tracking issue: rust-lang#56071

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Apr 24, 2025

Rollup merge of rust-lang#138282 - beetrees:repr128-not-ffi-safe, r=oli-obk

Add #[repr(u128)]/#[repr(i128)] enums to improper_ctypes_definitions

This makes them warn whenever a plain u128/i128 would. If the lang team decides to merge rust-lang#137306 then this can be reverted.

Tracking issue: rust-lang#56071

Is there precedent for improper_ctypes being target-dependent? That seems problematic as the crate author seems likely to not see the diagnostic and users will not see it either when it occurs in a dependency.

AFAIK no: the closest would be AIX having a lint for when Rust's repr(C) struct layout doesn't match the C compiler's, but that's done via a separate uses_power_alignment lint (added in #135552).

note for posterity: this lint was found to be a pile of confusion that wasn't really addressing the right problem. I do not regret merging the lint because meandering down the confused path, despite proving slightly aimless, eventually surfaced the real problems. however, it should probably be considered an example to avoid following.

I do not think this fact should necessarily change anyone's decisions here, just making clear that it was using linting Rust source to solve a problem so deep it was instead about the compiler's model of the target. that problem lives in either rustc's backend crates or LLVM, depending on your perspective.

@rfcbot resolve update-per-nikic

We talked about this in the lang call today, with 3/5 present, and were happy to see this go forward. We noted that we should probably be more clear about the fact that while this lint is designed to catch bugs, it's not the definitive reference on what Rust considers to be ABI compatible with C.

🔔 This is now entering its final comment period, as per the review above. 🔔

it's not the definitive reference on what Rust considers to be ABI compatible with C.

It is, however, the only reference that exists for this at the moment, to my knowledge...

Labels

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

final-comment-period

In the final comment period and will be merged soon unless new substantive objections are raised.

I-lang-radar

Items that are on lang's radar and will need eventual work or consideration.

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-lang

Relevant to the language team, which will review and decide on the PR/issue.