Ensure shift instrinsic arguments match width of compiler-rt's. #522

cr1901 · 2023-03-26T05:53:03Z

This fixes #520. I tested this locally and confirmed this fixes my issue, along with the additional changes listed in the below sections just for testing.

I realized while working on this patch that as far as compiler-builtins is concerned, only the shift instrinsics are affected by 16-bit vs 32-bit mismatch. This dates back to a patch by @aykevl in 2020.

It is very possible that the msp430 backend generates code w/ incorrect width for other instrinsics changed in the above patch. However, I don't know a good way to sift these out w/ tests, Rust or otherwise. Perhaps rust-lang/rust#107612 could be useful here? mspdebug has a simulator; it's how I was able to get a test case for #520.

`compiler-builtins` patches

diff --git a/src/lib.rs b/src/lib.rs
index 71f249c..dabe076 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -14,6 +14,7 @@
 #![no_builtins]
 #![no_std]
 #![allow(unused_features)]
+#![allow(unexpected_cfgs)]
 // We use `u128` in a whole bunch of places which we currently agree with the
 // compiler on ABIs and such, so we should be "good enough" for now and changes
 // to the `u128` ABI will be reflected here.

Without this, for some reason Rust bails with -D warnings, due to rust-lang/rust#96472 when I'm using compiler-builtins as a path dep (but not when using a crates.io dep... huh...).

`rust` patches

diff --git a/Cargo.lock b/Cargo.lock
index 449f0c73588..c8a69ff7d5a 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -868,9 +868,7 @@ dependencies = [
 
 [[package]]
 name = "compiler_builtins"
-version = "0.1.87"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f867ce54c09855ccd135ad4a50c777182a0c7af5ff20a8f537617bd648b10d50"
+version = "0.1.90"
 dependencies = [
  "cc",
  "rustc-std-workspace-core",
diff --git a/Cargo.toml b/Cargo.toml
index 15cbb2659c9..8fa9b040bfa 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -104,6 +104,12 @@ gimli.debug = 0
 miniz_oxide.debug = 0
 object.debug = 0
 
+[profile.release.package.core]
+opt-level = 's'
+
+[profile.release.package.alloc]
+opt-level = 's'
+
 [patch.crates-io]
 # See comments in `src/tools/rustc-workspace-hack/README.md` for what's going on
 # here
@@ -114,6 +120,7 @@ rustc-workspace-hack = { path = 'src/tools/rustc-workspace-hack' }
 rustc-std-workspace-core = { path = 'library/rustc-std-workspace-core' }
 rustc-std-workspace-alloc = { path = 'library/rustc-std-workspace-alloc' }
 rustc-std-workspace-std = { path = 'library/rustc-std-workspace-std' }
+compiler_builtins = { version="0.1.87", path = '../compiler-builtins' }
 
 [patch."https://github.com/rust-lang/rust-clippy"]
 clippy_lints = { path = "src/tools/clippy/clippy_lints" }
diff --git a/library/backtrace b/library/backtrace
--- a/library/backtrace
+++ b/library/backtrace
@@ -1 +1 @@
-Subproject commit 07872f28cd8a65c3c7428811548dc85f1f2fb05b
+Subproject commit 07872f28cd8a65c3c7428811548dc85f1f2fb05b-dirty

Use local compiler-builtins with my patch, work around some unrelated LLVM assertion failures at opt-level=3.

cr1901 · 2023-03-26T06:05:39Z

https://github.com/rust-lang/compiler-builtins/actions/runs/4523089026/jobs/7965979554?pr=522#step:8:941

Interesting... well, floatuntisf does use shifts, but without having access to the assembly, I can't tell why these conversions weren't a noop on 32-bit platforms and above. Ahhh well, not solving that tonight, will take a look when I have time.

Just to be clear, the tests pass on my local Linux box:

Debug

william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$ cargo test --manifest-path testcrate/Cargo.toml
   Compiling compiler_builtins v0.1.90 (/home/william/Projects/toolchains/compiler-builtins)
   Compiling rand_core v0.6.3
   Compiling rand_xoshiro v0.6.0
   Compiling testcrate v0.1.0 (/home/william/Projects/toolchains/compiler-builtins/testcrate)
    Finished test [unoptimized + debuginfo] target(s) in 2.12s
     Running tests/addsub.rs (target/debug/deps/addsub-5959085560820546)

running 2 tests
test float_addsub ... ok
test addsub ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.11s

     Running tests/aeabi_memclr.rs (target/debug/deps/aeabi_memclr-3efcc0b49b7b28a2)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/aeabi_memcpy.rs (target/debug/deps/aeabi_memcpy-1530b77bb1c70102)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/aeabi_memset.rs (target/debug/deps/aeabi_memset-5b979390d77eebf1)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/cmp.rs (target/debug/deps/cmp-bdb1f9aa297cd59f)

running 1 test
test float_comparisons ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

     Running tests/conv.rs (target/debug/deps/conv-1e647b35d259118f)

running 2 tests
test float_to_int ... ok
test int_to_float ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.08s

     Running tests/div_rem.rs (target/debug/deps/div_rem-fb17febba670c34a)

running 5 tests
test div_rem_si4 ... ok
test div_rem_di4 ... ok
test float_div ... ok
test divide_sparc ... ok
test div_rem_ti4 ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s

     Running tests/mem.rs (target/debug/deps/mem-d448b32093323f1c)

running 18 tests
test memcpy_10 ... ok
test memcpy_3 ... ok
test memmove_backward ... ok
test memmove_backward_aligned ... ok
test memcpy_big ... ok
test memmove_backward_misaligned_aligned_start ... ok
test memmove_backward_misaligned_nonaligned_start ... ok
test memmove_forward_aligned ... ok
test memmove_forward ... ok
test memmove_forward_misaligned_aligned_start ... ok
test memcmp_eq ... ok
test memmove_forward_misaligned_nonaligned_start ... ok
test memset_backward_aligned ... ok
test memset_backward_misaligned_aligned_start ... ok
test memset_backward_misaligned_nonaligned_start ... ok
test memset_nonzero ... ok
test memset_zero ... ok
test memcmp_ne ... ok

test result: ok. 18 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

     Running tests/misc.rs (target/debug/deps/misc-75d58197493a1295)

running 5 tests
test fuzz_values ... ok
test float_extend ... ok
test leading_zeros ... ok
test float_trunc ... ok
test float_pow ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

     Running tests/mul.rs (target/debug/deps/mul-73b2d7b870bf376a)

running 3 tests
test float_mul ... ok
test mul ... ok
test overflowing_mul ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.08s

     Running tests/shift.rs (target/debug/deps/shift-9f24e6fb359ea9b4)

running 1 test
test shift ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Release

william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$ cargo test --release --manifest-path testcrate/Cargo.toml
   Compiling compiler_builtins v0.1.90 (/home/william/Projects/toolchains/compiler-builtins)
   Compiling rand_core v0.6.3
   Compiling rand_xoshiro v0.6.0
   Compiling testcrate v0.1.0 (/home/william/Projects/toolchains/compiler-builtins/testcrate)
    Finished release [optimized] target(s) in 2.26s
     Running tests/addsub.rs (target/release/deps/addsub-6040db46b7b3d62f)

running 2 tests
test addsub ... ok
test float_addsub ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.16s

     Running tests/aeabi_memclr.rs (target/release/deps/aeabi_memclr-7b237f253af46b40)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/aeabi_memcpy.rs (target/release/deps/aeabi_memcpy-9de7a0ba3047dcaf)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/aeabi_memset.rs (target/release/deps/aeabi_memset-c57259d4a8983005)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/cmp.rs (target/release/deps/cmp-d24b5872f7868635)

running 1 test
test float_comparisons ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.17s

     Running tests/conv.rs (target/release/deps/conv-d44c657982f35675)

running 2 tests
test float_to_int ... ok
test int_to_float ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.44s

     Running tests/div_rem.rs (target/release/deps/div_rem-8e106040aed55aef)

running 5 tests
test div_rem_di4 ... ok
test div_rem_si4 ... ok
test divide_sparc ... ok
test div_rem_ti4 ... ok
test float_div ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.20s

     Running tests/mem.rs (target/release/deps/mem-86db6098578ed8f8)

running 18 tests
test memcmp_eq ... ok
test memcpy_10 ... ok
test memcpy_3 ... ok
test memcpy_big ... ok
test memmove_backward_aligned ... ok
test memmove_backward ... ok
test memmove_backward_misaligned_aligned_start ... ok
test memmove_backward_misaligned_nonaligned_start ... ok
test memmove_forward ... ok
test memmove_forward_aligned ... ok
test memmove_forward_misaligned_aligned_start ... ok
test memmove_forward_misaligned_nonaligned_start ... ok
test memset_backward_aligned ... ok
test memset_backward_misaligned_aligned_start ... ok
test memset_backward_misaligned_nonaligned_start ... ok
test memset_nonzero ... ok
test memset_zero ... ok
test memcmp_ne ... ok

test result: ok. 18 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/misc.rs (target/release/deps/misc-e8e6f8cc64e9ef62)

running 5 tests
test fuzz_values ... ok
test leading_zeros ... ok
test float_extend ... ok
test float_trunc ... ok
test float_pow ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.24s

     Running tests/mul.rs (target/release/deps/mul-9ca5f7fce34b6134)

running 3 tests
test mul ... ok
test float_mul ... ok
test overflowing_mul ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.18s

     Running tests/shift.rs (target/release/deps/shift-89e89344ee2dea1d)

running 1 test
test shift ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$

Amanieu · 2023-03-29T21:43:32Z

Can you rebase now that #524 is merged, which should fix CI?

…s si_int).

cr1901 · 2023-03-29T22:12:16Z

@Amanieu Done, and CI is green :D!

aykevl · 2023-03-30T01:07:35Z

src/int/shift.rs

+    pub extern "C" fn __ashldi3(a: u64, b: core::ffi::c_uint) -> u64 {
+        a.ashl(b as u32)


Drive by comment:

While this appears to follow the calling convention that LLVM uses, it does not follow the calling convention that avr-gcc uses: https://godbolt.org/z/azq4749hd
b is only a single byte on avr-gcc.
(I also see now that the C implementation in compiler-rt has the same bug, but only realized it now).

AIUI, these code paths are skipped when compiling for avr, precisely because there's a pass which gets rid of these types of shifts in LLVM. So I made no accommodations for its nonstandard calling conventions in this PR.

That being said, I don't know what a proper fix should look like that fixes avr, msp430, and everything-else.

because there's a pass which gets rid of these types of shifts in LLVM

I know, because I wrote that pass ;)
But it only works for 32-bit values.__ashldi3 is a 64-bit shift function, which is emitted as a regular compiler-rt call. Proof: https://godbolt.org/z/zqWfE6ndq

Anyway, it doesn't really matter as LLVM uses i16 and not i8 like avr-gcc does. This PR would only be buggy for avr-gcc, right now it is safe with LLVM (but this could in theory change as i8 is more efficient).

Minddump for future-me follows:

b is only a single byte on avr-gcc.

According to gcc docs, __ashldi3's b argument is supposed to take an int, which must be at least 16 bits. But indeed, your GCC code snippet is only sending a byte to __ashldi3, while the LLVM version sends 16 bits (the clr r17 line is clearing the top-most bits).

On the other hand, gcc's docs also says __ashldi3 is supposed to return a long, and __ashlti3 is the function that is supposed to return a long long.

On the other _other hand, compiler-rt doesn't seem to support the t versions of functions, and tends to hardcode a 64/32-bit int in places gcc's builtins do not¹. In addition to compiler-rt uses unsigned in args where gcc's builtins are signed.

In other words, it seems like both gcc compiler-rt builtins tend to do their own things and aren't directly compatible with each other. avr using a byte in for the gcc __ashldi3 b arg, but 2 bytes for compiler-rt's __ashldi3 b arg is just one example of this. It's not going to be fun :P. Unifying compiler-rt's and gcc's builtins in terms of compiler_builtins might be necessary due to the gcc backend though.

The shift functions' b args notwithstanding :P.

But it only works for 32-bit values. __ashldi3 is a 64-bit shift function, which is emitted as a regular compiler-rt call.

How does this not break for Rust code, where all the shift functions are skipped for AVR (avr_skip)?

Update compiler-builtins to 0.1.91 to bring in msp430 shift primitive… … fixes. This fixes unsoundness on MSP430 where `compiler-builtins` and LLVM didn't agree on the width of the shift amount argument of the shifting primitives (4 bytes vs 2 bytes). See rust-lang/compiler-builtins#522 for more details.

Amanieu mentioned this pull request Mar 28, 2023

'LLVM ERROR: Expected a constant shift amount!' when compiling for avr-atmega328p #523

Closed

Ensure shift instrinsic arguments match width of compiler-rt's (int v…

038dee5

…s si_int).

cr1901 force-pushed the msp-shift branch from ebbab46 to 038dee5 Compare March 29, 2023 22:06

Amanieu merged commit 93d4c78 into rust-lang:master Mar 29, 2023

aykevl reviewed Mar 30, 2023

View reviewed changes

cr1901 mentioned this pull request Mar 30, 2023

Update compiler-builtins to 0.1.91 to bring in msp430 shift primitive… rust-lang/rust#109756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure shift instrinsic arguments match width of compiler-rt's. #522

Ensure shift instrinsic arguments match width of compiler-rt's. #522

cr1901 commented Mar 26, 2023

cr1901 commented Mar 26, 2023 •

edited

Loading

Amanieu commented Mar 29, 2023

cr1901 commented Mar 29, 2023

aykevl Mar 30, 2023

cr1901 Mar 30, 2023 •

edited

Loading

aykevl Mar 30, 2023

cr1901 Mar 30, 2023

cr1901 Mar 30, 2023

		pub extern "C" fn __ashldi3(a: u64, b: core::ffi::c_uint) -> u64 {
		a.ashl(b as u32)

Ensure shift instrinsic arguments match width of compiler-rt's. #522

Ensure shift instrinsic arguments match width of compiler-rt's. #522

Conversation

cr1901 commented Mar 26, 2023

compiler-builtins patches

rust patches

cr1901 commented Mar 26, 2023 • edited Loading

Debug

Release

Amanieu commented Mar 29, 2023

cr1901 commented Mar 29, 2023

aykevl Mar 30, 2023

Choose a reason for hiding this comment

cr1901 Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

aykevl Mar 30, 2023

Choose a reason for hiding this comment

cr1901 Mar 30, 2023

Choose a reason for hiding this comment

cr1901 Mar 30, 2023

Choose a reason for hiding this comment

`compiler-builtins` patches

`rust` patches

cr1901 commented Mar 26, 2023 •

edited

Loading

cr1901 Mar 30, 2023 •

edited

Loading