Skip to content

Bit-shifts with dynamic RHSs are broken on AVR on large numbers (u64, u128, i64, i128) #106135

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

Closed
Patryk27 opened this issue Dec 25, 2022 · 2 comments
Labels
C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.)

Comments

@Patryk27
Copy link
Contributor

Patryk27 commented Dec 25, 2022

Bit-shifts where the right-hand side is of statically unknown value yield spurious results - e.g.:

#[arduino_hal::entry]
fn main() -> ! {
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);
    let mut serial = arduino_hal::default_serial!(dp, pins, 57600);

    let x = 1;
    let y = hint::black_box(1);
    let val: u64 = x << y;

    for b in val.to_le_bytes() {
        _ = ufmt::uwrite!(&mut serial, "{} ", b);
    }

    _ = ufmt::uwriteln!(&mut serial, "");

    loop {
        //
    }
}

... prints:

0 0 0 4 0 0 0 0

For u128 we get something even wilder:

1 0 0 0 1 0 0 0 1 0 0 0 0 0 0 0

Smaller types, such as u32, seem to behave correctly:

2 0 0 0

My rough guess lays in the vicinity of AVRShiftExpand::expand() or compiler-builtins (ABI mismatch?), but I'm yet to confirm it.

Note that this is mostly an informational report - I'll try to fix this bug;

@Patryk27 Patryk27 added the C-bug Category: This is a bug. label Dec 25, 2022
@Patryk27
Copy link
Contributor Author

kk, it's ABI mismatch - sprinkling #[avr_skip] to a few functions in compiler-builtins/src/int/shift.rs seems to help.

Somewhat unfortunately, avr-gcc doesn't provide 128-bit versions, so adding those #[avr_skip] breaks u128 shifts (undefined reference to '__ashlti3'); maybe we could somehow expose the correct ABI from LLVM's side, so that the 128-bit versions could be provided by compiler-builtins, we'll see.

@workingjubilee workingjubilee added the O-AVR Target: AVR processors (ATtiny, ATmega, etc.) label Feb 5, 2023
@Patryk27
Copy link
Contributor Author

Current rustc already contains the fix (it went together with compiler-builtins 0.1.86), so lemme close the issue 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants