For msp430, `__ashldi3` takes a 16-bit integer as the second argument in LLVM's `compiler-rt`. Probably affects other builtins.

[cr1901]

I've created a test case that panics on msp430 in debug/release mode:

fn do_shift(val: &u64, amt: &u8) -> u64 {
    // compiler_builtins::int::shift::__ashldi3(*val, *amt as u32)
    *val << *amt
}

fn shift_test() {
    let my_u64: u64 = 1;
    let my_shift: u8 = 0;

    if black_box(do_shift(black_box(&my_u64), black_box(&my_shift))) != 1 {
        panic!()
    }
}

If I call compiler_builtins::int::shift::__ashldi3(*val, *amt as u32) directly, shift_test does not panic. Relevant assembly code:

.Ltmp6:
	.loc	1 46 18 is_stmt 1
	mov	6(r12), r15
	mov	4(r12), r14
	mov	2(r12), r13
	mov	0(r12), r12
	mov.b	0(r11), r11
.Ltmp7:
	.loc	1 38 5
	mov	r11, 0(r1)
	clr	2(r1)
	call	#_ZN17compiler_builtins3int5shift9__ashldi317h573dd44357b45ab3E

However, if I let LLVM lower the shift to a builtin, shift_test tends to panic:

.Ltmp7:
	.loc	1 46 18 is_stmt 1
	mov.b	0(r12), r12
.Ltmp8:
	.loc	1 39 5
	and	#63, r12
	mov	r12, 0(r1)
.Ltmp9:
	.loc	1 46 18
	mov	0(r15), r12
	mov	2(r15), r13
	mov	4(r15), r14
	mov	6(r15), r15
.Ltmp10:
	.loc	1 39 5
	call	#__ashldi3

Notice that the call to __ashldi3 only sets up one 64-bit worth and a 16-bit int's worth of argument. This is correct for __ashldi3 as specified in compiler-rt. However, we call the compiler-builtins version in both cases (compiled with -Zbuild-std=core), which excepts a 32-bit int's worth of argument. The second code snippet does not do this, and therefore when compiler_builtins::int::shift::__ashldi3 is called it reads garbage for the top 16-bits of the shl argument. This causes subtle breakage and pain, possibly many instructions away from the call :).

AVR doesn't appear to have this problem because apparently there's an dedicated pass to ensuring that calls to __ashldi3 and friends aren't emitted into assembly files. MSP430 doesn't have such a pass. Which makes me confused as to why #513 was needed in the first place, but that's for another time.

This leaves msp430 as the only platform which would use at the very least the shift builtins where sizeof(int) isn't 4. I'm not sure how to fix this:

• The code emission for LLVM is correct for C/C++ code.
  • Changing MSP430 to a 32-bit int will never be accepted on the LLVM side.
• On the other hand, going through every single function that uses an int where this crate uses a u32 and adding conditional compilation seems invasive.
• On the other, other hand, I don't want to go the build-std-features route to disable compiler_builtins because I'm trying to remove nightly features from the msp430 backend, not add them.
• __ashldi3 does exist in libgcc:

  $ nm /opt/toolchains/lib/gcc/msp430-elf/9.3.1/libgcc.a 2> /dev/null | grep __ashldi3
  00000000 T __ashldi3
  

• ... but I can't confirm that all affected builtins (ones using int args) are in libgcc for MSP430. I'd rather see compiler-builtins correctly support 16-bit int input arguments rather than hardcode a 32-bit one. Maybe adding an msp430_expand wrapper is possible- i.e. expand 16-bit ints to 32-bit before sending to __ashldi3? That seems like it could be done in a semver-compat manner somehow?

[Amanieu]

If this is just a matter of int being i16 instead of i32 then the best way to do this would be to change all builtins to use core::ffi::c_int instead of i32 for their arguments where the C equivalent uses int. This should have no effect on most targets but should fix msp430.

[cr1901]

Doing this would technically be a breaking change according to semver, as calling compiler_builtins primitives directly from msp430 code works just fine w/ the current function signatures.

I personally haven't used compiler_builtins primitives directly (except for testing), but if someone else does in their crate and compiles for --target msp430-none-elf, changing from i32 to c_int will fail to compile/break, correct?

Does this warrant a bump to 0.2 or do we accept the minimal breakage and bump to 0.1.89?

[Amanieu]

compiler_builtins is an internal implementation detail of Rust and isn't meant to be called directly. Also Rust's semver policy explicitly allows breaking changes if it fixes a soundness bug.

[cr1901]

Alright, I'll take a look this coming week and prepare a patch then :).

[Patryk27]

It seems the same issue affects floating-point intrinsics for AVR (#711 (comment)); I'm investigating the issue and I'll try to prepare a PR.