Create optionally-available __int128 typedefs and use them for ARM64 definitions. #2719

Gankra · 2022-03-09T06:43:40Z

Potentially fixes #2524, see the comments in the patch for details.

rust-highfive · 2022-03-09T06:43:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

Please see the contribution instructions for more information.

Gankra · 2022-03-09T06:46:39Z

See rust-lang/rust#54341 (comment) for details on my investigation into this.

I will happily fill in more OS/arches if anyone can run https://github.com/Gankra/abi-checker on that target and get full passes. I only have access to aarch64 Android (phone) and macOS (coworker). I'm kinda blindly assuming iOS is the same as macOS here.

Gankra · 2022-03-09T06:53:43Z

For context, properly defining these types is important for saving a crashing process' registers (i.e. when generating a breakpad-style minidump, which I am currently working on porting to Rust for Android).

src/fixed_width_ints.rs

Gankra · 2022-03-09T13:35:00Z

In the upstream issue nagisa notes that rust uniformly has this repr on all aarch64 targets -- but I'd still like to verify that the primary C compilers for those targets agree.

Amanieu

Great work! I think this was sorely needed.

src/unix/linux_like/android/b64/aarch64/mod.rs

Amanieu · 2022-03-11T16:56:27Z

src/fixed_width_ints.rs

+
+cfg_if! {
+    if #[cfg(all(target_arch = "aarch64",
+        any(target_os = "android", target_os="macos", target_os = "ios")))] {


I would feel comfortable just doing not(windows) here. That way all non-Windows platforms automatically get support and this is confirmed by the static assert.

Gankra · 2022-03-11T18:07:38Z

So since putting up this PR I spent a bit more time digging into what can be done to polyfill this type on x64 (since SysV actually passes structs the same as ints, in general). Unfortunately on x64 (linux, at least) clang and gcc actually disagree on the pass-on-stack convention for __int128:

https://twitter.com/Gankra_/status/1502320041315946499

x64 SysV fails to specify what "pushing" a value actually means and so clang packs __int128 while gcc properly aligns it. This can introduce padding and differences and completely desynchronize the offsets to args.

In discussing this on Twitter some Apple engineers also pointed out that clang actually has macos-specific ABI differences for backcompat with itself (details vague, no documentation for this in general). Similarly a coworker noted that stuff like this is a problem for clang plugins when the plugin and compiler weren't both consistently built by clang or gcc.

This now has me increasingly paranoid about claiming u128/i128 are ABI-compatible on ARM64 without any way for me to properly compare gcc and clang both targetting these platforms. And also makes me less inclined to willy-nilly declare that Rust agrees with C compilers for "all non-windows OSes". Even asserting they agree on size and align isn't sufficient because the compilers can still disagree on how the value is pushed to the stack. :(

I'm not sure what libc's policy is on divergent impls for one platform, and how to declare one "canonical" over the others.

Unfortunately I can't really test this stuff out better -- the "gcc" on my phone's termux is just an alias for clang, and similarly I expect that will be the case for m1 macbooks.

If anyone has better access to arm64 platforms and can setup clang/gcc properly, they can test this out by checking out:

https://github.com/Gankra/abi-checker

and running

cargo run -- --tests ui128

This will compare CC to rustc and CC to itself. That is a good start for rust matching the "canonical" implementation.

To exhaustively try out all the combos of clang+gcc+rust to check if the ABI is actually consistent between all impls, you can do:

cargo run -- --tests ui128 --pairs clang_calls_gcc gcc_calls_clang rustc_calls_gcc gcc_calls_rustc rustc_calls_clang clang_calls_rustc clang_calls_clang gcc_calls_gcc rustc_calls_rustc

Otherwise you can manually do something like this godbolt.

Amanieu · 2022-03-12T16:14:11Z

I ran your ABI checker on AArch64 Linux, all the tests pass:

Final Results:
ui128::c::clang_calls_gcc        all 226/226 passed
ui128::c::gcc_calls_clang        all 226/226 passed
ui128::c::rustc_calls_gcc        all 226/226 passed
ui128::c::gcc_calls_rustc        all 226/226 passed
ui128::c::rustc_calls_clang      all 226/226 passed
ui128::c::clang_calls_rustc      all 226/226 passed
ui128::c::clang_calls_clang      all 226/226 passed
ui128::c::gcc_calls_gcc          all 226/226 passed
ui128::c::rustc_calls_rustc      all 226/226 passed
ui128::cdecl::rustc_calls_rustc  all 226/226 passed
ui128::stdcall::rustc_calls_rustc all 226/226 passed
ui128::fastcall::rustc_calls_rustc all 226/226 passed

2712 passed, 0 failed, 0 completely failed, 33 skipped

Gankra · 2022-03-12T23:03:32Z

Oh nice!! that helps my confidence with going forward with this a lot. I'll clean this up on monday.

Gankra · 2022-03-12T23:04:56Z

Also lmao @ rustc actually managing to do something on linux when you ask it to fastcall/stdcall with itself, I hadn't actually every tested that.

Gankra · 2022-03-15T18:07:32Z

Applied review requests and squashed.

Amanieu · 2022-03-17T10:59:16Z

@bors r+

bors · 2022-03-17T10:59:17Z

📌 Commit 3741d52 has been approved by Amanieu

bors · 2022-03-17T10:59:25Z

⌛ Testing commit 3741d52 with merge ca04d09...

Create optionally-available __int128 typedefs and use them for ARM64 definitions. Potentially fixes #2524, see the comments in the patch for details.

bors · 2022-03-17T11:15:03Z

💔 Test failed - checks-actions

Amanieu · 2022-03-17T12:20:52Z

src/fixed_width_ints.rs

+        /// is actually *passed* on the stack (clang underaligns it for
+        /// the same reason that rustc *never* properly aligns it).
+        const _ASSERT_128_COMPAT: () = {
+            assert!(std::mem::size_of::<__int128>() == SIZE_128);


This should use core instead of std.

Amanieu · 2022-03-17T12:21:44Z

src/unix/linux_like/android/b64/aarch64/mod.rs

@@ -55,6 +55,12 @@ s! {
        pub pc: u64,
        pub pstate: u64,
    }
+
+    pub struct user_fpsimd_struct {
+        pub vregs: [__uint128_t; 32],


This type needs to be imported:

Suggested change

pub vregs: [__uint128_t; 32],

pub vregs: [::__uint128_t; 32],

Gankra · 2022-03-18T00:16:06Z

ok!!! this one actually seems to work and satisfy the nightmare cfg macro rube-goldberg machine (remaining failures are the pre-existing ones in the repo itself).

bors · 2022-03-18T00:51:07Z

💔 Test failed - checks-actions

Gankra · 2022-03-18T16:52:03Z

@bors r+

bors · 2022-03-18T16:52:05Z

📌 Commit 127542f has been approved by Gankra

Create optionally-available __int128 typedefs and use them for ARM64 definitions. Potentially fixes #2524, see the comments in the patch for details.

bors · 2022-03-18T17:02:37Z

⌛ Testing commit 127542f with merge aeb1a4e...

bors · 2022-03-18T17:58:54Z

💔 Test failed - checks-actions

…definitions. This adds the following types to fixed_width_ints behind appropriate platform cfgs: * __int128 * __int128_t * __uint128 * __uint128_t and user_fpsimd_struct to arm64 android and linux.

Gankra · 2022-03-23T22:24:44Z

ok coming back to this with a fresh mind and the design of libc i think now has congealed into a coherent form for me. Hopefully this works now...

Gankra · 2022-03-23T22:30:22Z

@bors try

Create optionally-available __int128 typedefs and use them for ARM64 definitions. Potentially fixes #2524, see the comments in the patch for details.

bors · 2022-03-23T22:30:30Z

⌛ Trying commit adfa385 with merge 245c710...

Gankra · 2022-03-23T23:02:10Z

@bors r+

bors · 2022-03-23T23:02:11Z

📌 Commit adfa385 has been approved by Gankra

bors · 2022-03-23T23:02:32Z

⌛ Testing commit adfa385 with merge f00ea00...

bors · 2022-03-24T00:04:28Z

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Gankra
Pushing f00ea00 to master...

Two improvements of the latest i128 numbers change Two improvements of #2719: * First, a typo/grammar fix * Second, a fix to avoid usage of the anti pattern of `==` inside an `assert` macro. Since a few releases, `assert` is usable inside `const` expressions, but for the purpose of `==`, superior alternatives exist that also work at compile time but print the two values at a mismatch. `assert_eq` is not yet usable in const contexts due to formatting.

rust-highfive assigned Amanieu Mar 9, 2022

rust-highfive added the S-waiting-on-review label Mar 9, 2022

bjorn3 reviewed Mar 9, 2022

View reviewed changes

src/fixed_width_ints.rs Outdated Show resolved Hide resolved

Gankra force-pushed the fpsimd branch from 2498ceb to b425ad7 Compare March 9, 2022 13:28

Amanieu reviewed Mar 11, 2022

View reviewed changes

Gankra force-pushed the fpsimd branch from b425ad7 to 7c3fdf3 Compare March 15, 2022 18:07

Gankra changed the title ~~WIP attempt to implement user_fpsimd_struct.~~ Create optionally-available __int128 typedefs and use them for ARM64 definitions. Mar 15, 2022

Gankra mentioned this pull request Mar 15, 2022

arm64 android missing a bunch of ptrace apis? nix-rust/nix#1678

Open

Gankra force-pushed the fpsimd branch from 7c3fdf3 to 3741d52 Compare March 15, 2022 20:25

Gankra mentioned this pull request Mar 16, 2022

WIP implement some GETREGSET functionality for x64 and aarch64 nix-rust/nix#1679

Open

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 17, 2022

bors added a commit that referenced this pull request Mar 17, 2022

Auto merge of #2719 - Gankra:fpsimd, r=Amanieu

ca04d09

Create optionally-available __int128 typedefs and use them for ARM64 definitions. Potentially fixes #2524, see the comments in the patch for details.

bors added S-waiting-on-review and removed S-waiting-on-bors labels Mar 17, 2022

Amanieu reviewed Mar 17, 2022

View reviewed changes

Gankra force-pushed the fpsimd branch from 3741d52 to c0d7485 Compare March 17, 2022 15:31

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 18, 2022

bors added a commit that referenced this pull request Mar 18, 2022

Auto merge of #2719 - Gankra:fpsimd, r=Gankra

aeb1a4e

Create optionally-available __int128 typedefs and use them for ARM64 definitions. Potentially fixes #2524, see the comments in the patch for details.

bors added S-waiting-on-review and removed S-waiting-on-bors labels Mar 18, 2022

Create optionally-available __int128 typedefs and use them for ARM64 …

adfa385

…definitions. This adds the following types to fixed_width_ints behind appropriate platform cfgs: * __int128 * __int128_t * __uint128 * __uint128_t and user_fpsimd_struct to arm64 android and linux.

Gankra force-pushed the fpsimd branch from 127542f to adfa385 Compare March 23, 2022 22:21

bors added a commit that referenced this pull request Mar 23, 2022

Auto merge of #2719 - Gankra:fpsimd, r=<try>

245c710

Create optionally-available __int128 typedefs and use them for ARM64 definitions. Potentially fixes #2524, see the comments in the patch for details.

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 23, 2022

bors merged commit f00ea00 into rust-lang:master Mar 24, 2022

Gankra mentioned this pull request Mar 24, 2022

i128 / u128 are not compatible with C's definition. rust-lang/rust#54341

Closed

est31 mentioned this pull request Mar 24, 2022

Two improvements of the latest i128 numbers change #2732

Merged

Gankra mentioned this pull request Apr 5, 2022

0.2.122 Release #2739

Merged

ojeda mentioned this pull request Jan 31, 2023

rustc wanted features & bugfixes Rust-for-Linux/linux#355

Open

51 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create optionally-available __int128 typedefs and use them for ARM64 definitions. #2719

Create optionally-available __int128 typedefs and use them for ARM64 definitions. #2719

Gankra commented Mar 9, 2022

rust-highfive commented Mar 9, 2022

Gankra commented Mar 9, 2022

Gankra commented Mar 9, 2022

Gankra commented Mar 9, 2022

Amanieu left a comment

Amanieu Mar 11, 2022

Gankra commented Mar 11, 2022

Amanieu commented Mar 12, 2022

Gankra commented Mar 12, 2022

Gankra commented Mar 12, 2022

Gankra commented Mar 15, 2022

Amanieu commented Mar 17, 2022

bors commented Mar 17, 2022

bors commented Mar 17, 2022

bors commented Mar 17, 2022

Amanieu Mar 17, 2022

Amanieu Mar 17, 2022

Gankra commented Mar 18, 2022

bors commented Mar 18, 2022

Gankra commented Mar 18, 2022

bors commented Mar 18, 2022

bors commented Mar 18, 2022

bors commented Mar 18, 2022

Gankra commented Mar 23, 2022

Gankra commented Mar 23, 2022

bors commented Mar 23, 2022

Gankra commented Mar 23, 2022

bors commented Mar 23, 2022

bors commented Mar 23, 2022

bors commented Mar 24, 2022

	pub vregs: [__uint128_t; 32],
	pub vregs: [::__uint128_t; 32],

Create optionally-available __int128 typedefs and use them for ARM64 definitions. #2719

Create optionally-available __int128 typedefs and use them for ARM64 definitions. #2719

Conversation

Gankra commented Mar 9, 2022

rust-highfive commented Mar 9, 2022

Gankra commented Mar 9, 2022

Gankra commented Mar 9, 2022

Gankra commented Mar 9, 2022

Amanieu left a comment

Choose a reason for hiding this comment

Amanieu Mar 11, 2022

Choose a reason for hiding this comment

Gankra commented Mar 11, 2022

Amanieu commented Mar 12, 2022

Gankra commented Mar 12, 2022

Gankra commented Mar 12, 2022

Gankra commented Mar 15, 2022

Amanieu commented Mar 17, 2022

bors commented Mar 17, 2022

bors commented Mar 17, 2022

bors commented Mar 17, 2022

Amanieu Mar 17, 2022

Choose a reason for hiding this comment

Amanieu Mar 17, 2022

Choose a reason for hiding this comment

Gankra commented Mar 18, 2022

bors commented Mar 18, 2022

Gankra commented Mar 18, 2022

bors commented Mar 18, 2022

bors commented Mar 18, 2022

bors commented Mar 18, 2022

Gankra commented Mar 23, 2022

Gankra commented Mar 23, 2022

bors commented Mar 23, 2022

Gankra commented Mar 23, 2022

bors commented Mar 23, 2022

bors commented Mar 23, 2022

bors commented Mar 24, 2022