Skip to content
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

[AVR] integer division incorrectly yields result value 1 #82242

Closed
crclark96 opened this issue Feb 17, 2021 · 13 comments · Fixed by rust-lang/compiler-builtins#462
Closed

[AVR] integer division incorrectly yields result value 1 #82242

crclark96 opened this issue Feb 17, 2021 · 13 comments · Fixed by rust-lang/compiler-builtins#462
Labels
C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) requires-nightly This issue requires a nightly compiler in some way.

Comments

@crclark96
Copy link

I'm writing some code for arduino using the avr-hal crate, and when I print the result of a division operation, I'm getting the result value of 1 for almost every combination of numbers. Exceptions that I've found so far include when the divisor is a power of two, which I'm assuming is being optimized to a bitshift operation (this is not happening in my minimum code example, I do not know why but it happened in another context so I'm including this detail here in case it comes up again).

I tried this code:

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut pins = arduino_uno::Pins::new(
        dp.PORTB,
        dp.PORTC,
        dp.PORTD,
    );
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for x in (5..100).step_by(5) {
        for y in 1..5 {
            let z = x / y;
            ufmt::uwriteln!(&mut serial, "{} / {} = {}\r", x, y, z).void_unwrap();
        }
    }
    loop {}
}

Output:

5 / 1 = 1
5 / 2 = 1
5 / 3 = 1
5 / 4 = 1
10 / 1 = 1
10 / 2 = 1
10 / 3 = 1
10 / 4 = 1
-- snip --
90 / 1 = 1
90 / 2 = 1
90 / 3 = 1
90 / 4 = 1
95 / 1 = 1
95 / 2 = 1
95 / 3 = 1
95 / 4 = 1

Meta

rustc --version --verbose:

rustc 1.51.0-nightly (c2de47a9a 2021-01-06)
binary: rustc
commit-hash: c2de47a9aa4c9812884f341f1852e9c9610f5f7a
commit-date: 2021-01-06
host: x86_64-unknown-linux-gnu
release: 1.51.0-nightly

I'm using nightly-2021-01-07 since AVR support has been broken since then. The code has to be built with the --release flag due to this issue: rust-lang/compiler-builtins#400

Here's the repo with the code and configs for AVR: https://github.com/crclark96/avr-division-repro. I'm using an Arduino Uno board for this.

@crclark96 crclark96 added the C-bug Category: This is a bug. label Feb 17, 2021
@jonas-schievink jonas-schievink added O-AVR Target: AVR processors (ATtiny, ATmega, etc.) requires-nightly This issue requires a nightly compiler in some way. labels Feb 18, 2021
@kolbma
Copy link

kolbma commented Feb 26, 2021

Any idea what might be responsible for this?
I've tried

nightly-2020-10-25-x86_64-unknown-linux-gnu
nightly-2020-11-25-x86_64-unknown-linux-gnu
nightly-2020-12-07-x86_64-unknown-linux-gnu
nightly-2020-12-12-x86_64-unknown-linux-gnu
nightly-2021-01-07-x86_64-unknown-linux-gnu

and division doesn't work.
Could be possible that I have not used divisions in my rust-avr code before, but I can't believe that nobody has used it?
Is it in the compiler_builtins crate?

Wondering why such nasty bugs are not caught in the compiler build pipeline...

@jacobmischka
Copy link

jacobmischka commented Feb 26, 2021

The example above does reproduce for me, I receive all 1s, but I have been successful performing integer division in projects. The math here for driving a speaker, for example, does properly produce the correct tones on the same device I tested the above code on. I do not understand how both of those could be true, but it seems to be the case.

Edit: Actually, I think i32 (and i64 [and u64, see below]) >= 32-bit integers are the problem. Unfortunate that that's the default for int literals, lol. Unsigned integers u32s u16s and even i16s seem to work.

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;
use ufmt::uwriteln;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for x in (5u16..100u16).step_by(5) {
        for y in 1u16..5u16 {
            let z = x / y;
            uwriteln!(&mut serial, "{} / {} = {}\r", x, y, z).void_unwrap();
        }
    }

    loop {}
}
Output

5 / 1 = 5
5 / 2 = 2
5 / 3 = 1
5 / 4 = 1
10 / 1 = 10
10 / 2 = 5
10 / 3 = 3
10 / 4 = 2
15 / 1 = 15
15 / 2 = 7
15 / 3 = 5
15 / 4 = 3
20 / 1 = 20
20 / 2 = 10
20 / 3 = 6
20 / 4 = 5
25 / 1 = 25
25 / 2 = 12
25 / 3 = 8
25 / 4 = 6
30 / 1 = 30
30 / 2 = 15
30 / 3 = 10
30 / 4 = 7
35 / 1 = 35
35 / 2 = 17
35 / 3 = 11
35 / 4 = 8
40 / 1 = 40
40 / 2 = 20
40 / 3 = 13
40 / 4 = 10
45 / 1 = 45
45 / 2 = 22
45 / 3 = 15
45 / 4 = 11
50 / 1 = 50
50 / 2 = 25
50 / 3 = 16
50 / 4 = 12
55 / 1 = 55
55 / 2 = 27
55 / 3 = 18
55 / 4 = 13
60 / 1 = 60
60 / 2 = 30
60 / 3 = 20
60 / 4 = 15
65 / 1 = 65
65 / 2 = 32
65 / 3 = 21
65 / 4 = 16
70 / 1 = 70
70 / 2 = 35
70 / 3 = 23
70 / 4 = 17
75 / 1 = 75
75 / 2 = 37
75 / 3 = 25
75 / 4 = 18
80 / 1 = 80
80 / 2 = 40
80 / 3 = 26
80 / 4 = 20
85 / 1 = 85
85 / 2 = 42
85 / 3 = 28
85 / 4 = 21
90 / 1 = 90
90 / 2 = 45
90 / 3 = 30
90 / 4 = 22
95 / 1 = 95
95 / 2 = 47
95 / 3 = 31
95 / 4 = 23

Output with i16
5 / 1 = 5
5 / 2 = 2
5 / 3 = 1
5 / 4 = 1
10 / 1 = 10
10 / 2 = 5
10 / 3 = 3
10 / 4 = 2
15 / 1 = 15
15 / 2 = 7
15 / 3 = 5
15 / 4 = 3
20 / 1 = 20
20 / 2 = 10
20 / 3 = 6
20 / 4 = 5
25 / 1 = 25
25 / 2 = 12
25 / 3 = 8
25 / 4 = 6
30 / 1 = 30
30 / 2 = 15
30 / 3 = 10
30 / 4 = 7
35 / 1 = 35
35 / 2 = 17
35 / 3 = 11
35 / 4 = 8
40 / 1 = 40
40 / 2 = 20
40 / 3 = 13
40 / 4 = 10
45 / 1 = 45
45 / 2 = 22
45 / 3 = 15
45 / 4 = 11
50 / 1 = 50
50 / 2 = 25
50 / 3 = 16
50 / 4 = 12
55 / 1 = 55
55 / 2 = 27
55 / 3 = 18
55 / 4 = 13
60 / 1 = 60
60 / 2 = 30
60 / 3 = 20
60 / 4 = 15
65 / 1 = 65
65 / 2 = 32
65 / 3 = 21
65 / 4 = 16
70 / 1 = 70
70 / 2 = 35
70 / 3 = 23
70 / 4 = 17
75 / 1 = 75
75 / 2 = 37
75 / 3 = 25
75 / 4 = 18
80 / 1 = 80
80 / 2 = 40
80 / 3 = 26
80 / 4 = 20
85 / 1 = 85
85 / 2 = 42
85 / 3 = 28
85 / 4 = 21
90 / 1 = 90
90 / 2 = 45
90 / 3 = 30
90 / 4 = 22
95 / 1 = 95
95 / 2 = 47
95 / 3 = 31
95 / 4 = 23

However:

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;
use ufmt::uwriteln;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for x in (5i32..10i32).step_by(5) {
        for y in 1i32..5i32 {
            let z = x / y;
            uwriteln!(&mut serial, "{} / {} = {:?}\r", x, y, z.to_le_bytes()).void_unwrap();
        }
    }

    loop {}
}

Output:

5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
10 / 1 = [1, 0, 1, 0]
10 / 2 = [1, 0, 1, 0]
10 / 3 = [1, 0, 1, 0]
10 / 4 = [1, 0, 1, 0]
15 / 1 = [0, 0, 1, 0]
15 / 2 = [0, 0, 1, 0]
15 / 3 = [0, 0, 1, 0]
15 / 4 = [005, 0, 1, 0]
20 / 1 = [1, 0, 005, 1]
20 / 2 = [1, 0, 005, 3]
20 / 3 = [1, 0, 000, 007]
20 / 4 = [1, 0, 005, 05]
25 / 6913 = [1, 0, 1, 0, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 7, 00�, 00�, 00�, 00�, 0�, 0�, 0�, 0�, 0�, 0�, 0�, 0�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 0�, 0�, 0�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 0�, 0�, 00�, 0�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 0�, 00�, 0�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 1, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 00�, 00�, 0�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 0�, 0�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 0�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 8, 00�, 00�, 00�, 00�, 0�, 0�, 0�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 0�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 0�, 00�, 00�, 00�, 00�, 0�, 00�, 0�, 00�, 00�, 00�, 00�, 0�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 00�, 0�]

Not sure what's going on with that last one, lol.

Edit 2: u64s aren't happy either.

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;
use ufmt::uwriteln;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for x in (5u64..10u64).step_by(5) {
        for y in 1u64..5u64 {
            let z = x / y;
            uwriteln!(
                &mut serial,
                "{:?} / {:?} = {:?}\r",
                x.to_le_bytes(),
                y.to_le_bytes(),
                z.to_le_bytes()
            )
            .void_unwrap();
        }
    }

    loop {}
}

Output:

[5, 0, 0, 0, 0, 0, 0, 0] / [1, 0, 0, 0, 0, 0, 0, 0] = [0, 0, 0, 10, 0, 0, 0, 0]
[5, 0, 0, 0, 0, 0, 0, 0] / [2, 0, 0, 0, 0, 0, 0, 0] = [8, 231, 8, 232, 8, 231, 8, 0]
[5, 0, 0, 0, 0, 0, 0, 0] / [3, 0, 0, 0, 0, 0, 0, 0] = [8, 231, 8, 232, 8, 231, 8, 50]
[5, 0, 0, 0, 0, 0, 0, 0] / [4, 0, 0, 0, 0, 0, 0, 0] = [8, 231, 8, 232, 8, 231, 8, 50]

Hangs after this point.

Edit 3: I wonder, is that data-layout we're using from the avr-hal repo (e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8) incorrect for i32 and i64?

@kolbma
Copy link

kolbma commented Feb 27, 2021

Strange, here u8 and u16 is fine. But u32 definitively not. I've not checked the signed variants.
It looks like memory corruption.
I've got also crashes when writing the result of the division to the usart with hardcoded u32, with u16 there is no problem.

@jacobmischka
Copy link

Oh, I didn't include a u32, oops, I meant to. Yeah, u32 uDisplay seems broken, but to_le/be/ne_bytes() works around that sort of.

@crclark96
Copy link
Author

Just a heads up I filed this issue for the ufmt crate when printing certain numbers, so using to_le/be/ne_bytes() seems to be the most reliable for now

japaric/ufmt#28

@jacobmischka
Copy link

jacobmischka commented Feb 27, 2021

Strange, here u8 and u16 is fine. But u32 definitively not. I've not checked the signed variants.
It looks like memory corruption.
I've got also crashes when writing the result of the division to the usart with hardcoded u32, with u16 there is no problem.

You're right, they don't work with u32 inputs. The reason why I was able to get it to work in my project I linked above is because they actually were u16s, just casted to u32.

    for x in (5u32..100u32).step_by(5) {
        for y in 1u32..5u32 {
            let z: u32 = x / y;
            uwriteln!(
                &mut serial,
                "{:?} / {:?} = {:?}\r",
                x.to_le_bytes(),
                y.to_le_bytes(),
                z.to_le_bytes()
            )
            .void_unwrap();
        }
    }
Output:
[5, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
[5, 0, 0, 0] / [2, 0, 0, 0] = [1, 0, 1, 0]
[5, 0, 0, 0] / [3, 0, 0, 0] = [1, 0, 1, 0]
[5, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]
[10, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
[10, 0, 0, 0] / [2, 0, 0, 0] = [1, 0, 1, 0]
[10, 0, 0, 0] / [3, 0, 0, 0] = [1, 0, 1, 0]
[10, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]
[15, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
[15, 0, 0, 0] / [2, 0, 0, 0] = [1, 0, 1, 0]
[15, 0, 0, 0] / [3, 0, 0, 0] = [1, 0, 1, 0]
[15, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]
[20, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
...
[80, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]
[85, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
[85, 0, 0, 0] / [2, 0, 0, 0] = [1, 0, 1, 0]
[85, 0, 0, 0] / [3, 0, 0, 0] = [1, 0, 1, 0]
[85, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]
[90, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
[90, 0, 0, 0] / [2, 0, 0, 0] = [1, 0, 1, 0]
[90, 0, 0, 0] / [3, 0, 0, 0] = [1, 0, 1, 0]
[90, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]
[95, 0, 0, 0] / [1, 0, 0, 0] = [1, 0, 1, 0]
[95, 0, 0, 0] / [2, 0, 0, 0] = [1, 0, 1, 0]
[95, 0, 0, 0] / [3, 0, 0, 0] = [1, 0, 1, 0]
[95, 0, 0, 0] / [4, 0, 0, 0] = [1, 0, 1, 0]

However:

    for x in (5u16..100u16).step_by(5) {
        for y in 1u16..5u16 {
            let z: u32 = x as u32 / y as u32;
            uwriteln!(&mut serial, "{} / {} = {:?}\r", x, y, z.to_le_bytes()).void_unwrap();
        }
    }
Output:
5 / 1 = [5, 0, 0, 0]
5 / 2 = [2, 0, 0, 0]
5 / 3 = [1, 0, 0, 0]
5 / 4 = [1, 0, 0, 0]
10 / 1 = [10, 0, 0, 0]
10 / 2 = [5, 0, 0, 0]
10 / 3 = [3, 0, 0, 0]
10 / 4 = [2, 0, 0, 0]
15 / 1 = [15, 0, 0, 0]
15 / 2 = [7, 0, 0, 0]
15 / 3 = [5, 0, 0, 0]
15 / 4 = [3, 0, 0, 0]
20 / 1 = [20, 0, 0, 0]
20 / 2 = [10, 0, 0, 0]
20 / 3 = [6, 0, 0, 0]
20 / 4 = [5, 0, 0, 0]
25 / 1 = [25, 0, 0, 0]
25 / 2 = [12, 0, 0, 0]
25 / 3 = [8, 0, 0, 0]
25 / 4 = [6, 0, 0, 0]
30 / 1 = [30, 0, 0, 0]
30 / 2 = [15, 0, 0, 0]
30 / 3 = [10, 0, 0, 0]
30 / 4 = [7, 0, 0, 0]
35 / 1 = [35, 0, 0, 0]
35 / 2 = [17, 0, 0, 0]
35 / 3 = [11, 0, 0, 0]
35 / 4 = [8, 0, 0, 0]
40 / 1 = [40, 0, 0, 0]
40 / 2 = [20, 0, 0, 0]
40 / 3 = [13, 0, 0, 0]
40 / 4 = [10, 0, 0, 0]
45 / 1 = [45, 0, 0, 0]
45 / 2 = [22, 0, 0, 0]
45 / 3 = [15, 0, 0, 0]
45 / 4 = [11, 0, 0, 0]
50 / 1 = [50, 0, 0, 0]
50 / 2 = [25, 0, 0, 0]
50 / 3 = [16, 0, 0, 0]
50 / 4 = [12, 0, 0, 0]
55 / 1 = [55, 0, 0, 0]
55 / 2 = [27, 0, 0, 0]
55 / 3 = [18, 0, 0, 0]
55 / 4 = [13, 0, 0, 0]
60 / 1 = [60, 0, 0, 0]
60 / 2 = [30, 0, 0, 0]
60 / 3 = [20, 0, 0, 0]
60 / 4 = [15, 0, 0, 0]
65 / 1 = [65, 0, 0, 0]
65 / 2 = [32, 0, 0, 0]
65 / 3 = [21, 0, 0, 0]
65 / 4 = [16, 0, 0, 0]
70 / 1 = [70, 0, 0, 0]
70 / 2 = [35, 0, 0, 0]
70 / 3 = [23, 0, 0, 0]
70 / 4 = [17, 0, 0, 0]
75 / 1 = [75, 0, 0, 0]
75 / 2 = [37, 0, 0, 0]
75 / 3 = [25, 0, 0, 0]
75 / 4 = [18, 0, 0, 0]
80 / 1 = [80, 0, 0, 0]
80 / 2 = [40, 0, 0, 0]
80 / 3 = [26, 0, 0, 0]
80 / 4 = [20, 0, 0, 0]
85 / 1 = [85, 0, 0, 0]
85 / 2 = [42, 0, 0, 0]
85 / 3 = [28, 0, 0, 0]
85 / 4 = [21, 0, 0, 0]
90 / 1 = [90, 0, 0, 0]
90 / 2 = [45, 0, 0, 0]
90 / 3 = [30, 0, 0, 0]
90 / 4 = [22, 0, 0, 0]
95 / 1 = [95, 0, 0, 0]
95 / 2 = [47, 0, 0, 0]
95 / 3 = [31, 0, 0, 0]
95 / 4 = [23, 0, 0, 0]

Edit: Interestingly, the mere presence of a u32 constant doesn't seem to break the division:

#![no_std]
#![no_main]

extern crate panic_halt;

use arduino_uno::prelude::*;
use ufmt::uwriteln;

const BIG_NUMBER: u32 = 16_000;

#[arduino_uno::entry]
fn main() -> ! {
    let dp = arduino_uno::Peripherals::take().unwrap();

    let mut pins = arduino_uno::Pins::new(dp.PORTB, dp.PORTC, dp.PORTD);
    let mut serial = arduino_uno::Serial::new(
        dp.USART0,
        pins.d0,
        pins.d1.into_output(&mut pins.ddr),
        57600.into_baudrate(),
    );

    for x in (5u16..100u16).step_by(5) {
        for y in 1u16..5u16 {
            let z: u32 = BIG_NUMBER / x as u32 / y as u32;
            uwriteln!(&mut serial, "{} / {} = {:?}\r", x, y, z.to_le_bytes()).void_unwrap();
        }
    }

    loop {}
}
Output:
5 / 1 = [128, 12, 0, 0]
5 / 2 = [64, 6, 0, 0]
5 / 3 = [42, 4, 0, 0]
5 / 4 = [32, 3, 0, 0]
10 / 1 = [64, 6, 0, 0]
10 / 2 = [32, 3, 0, 0]
10 / 3 = [21, 2, 0, 0]
10 / 4 = [144, 1, 0, 0]
15 / 1 = [42, 4, 0, 0]
15 / 2 = [21, 2, 0, 0]
15 / 3 = [99, 1, 0, 0]
15 / 4 = [10, 1, 0, 0]
20 / 1 = [32, 3, 0, 0]
20 / 2 = [144, 1, 0, 0]
20 / 3 = [10, 1, 0, 0]
20 / 4 = [200, 0, 0, 0]
25 / 1 = [128, 2, 0, 0]
25 / 2 = [64, 1, 0, 0]
25 / 3 = [213, 0, 0, 0]
25 / 4 = [160, 0, 0, 0]
30 / 1 = [21, 2, 0, 0]
30 / 2 = [10, 1, 0, 0]
30 / 3 = [177, 0, 0, 0]
30 / 4 = [133, 0, 0, 0]
35 / 1 = [201, 1, 0, 0]
35 / 2 = [228, 0, 0, 0]
35 / 3 = [152, 0, 0, 0]
35 / 4 = [114, 0, 0, 0]
40 / 1 = [144, 1, 0, 0]
40 / 2 = [200, 0, 0, 0]
40 / 3 = [133, 0, 0, 0]
40 / 4 = [100, 0, 0, 0]
45 / 1 = [99, 1, 0, 0]
45 / 2 = [177, 0, 0, 0]
45 / 3 = [118, 0, 0, 0]
45 / 4 = [88, 0, 0, 0]
50 / 1 = [64, 1, 0, 0]
50 / 2 = [160, 0, 0, 0]
50 / 3 = [106, 0, 0, 0]
50 / 4 = [80, 0, 0, 0]
55 / 1 = [34, 1, 0, 0]
55 / 2 = [145, 0, 0, 0]
55 / 3 = [96, 0, 0, 0]
55 / 4 = [72, 0, 0, 0]
60 / 1 = [10, 1, 0, 0]
60 / 2 = [133, 0, 0, 0]
60 / 3 = [88, 0, 0, 0]
60 / 4 = [66, 0, 0, 0]
65 / 1 = [246, 0, 0, 0]
65 / 2 = [123, 0, 0, 0]
65 / 3 = [82, 0, 0, 0]
65 / 4 = [61, 0, 0, 0]
70 / 1 = [228, 0, 0, 0]
70 / 2 = [114, 0, 0, 0]
70 / 3 = [76, 0, 0, 0]
70 / 4 = [57, 0, 0, 0]
75 / 1 = [213, 0, 0, 0]
75 / 2 = [106, 0, 0, 0]
75 / 3 = [71, 0, 0, 0]
75 / 4 = [53, 0, 0, 0]
80 / 1 = [200, 0, 0, 0]
80 / 2 = [100, 0, 0, 0]
80 / 3 = [66, 0, 0, 0]
80 / 4 = [50, 0, 0, 0]
85 / 1 = [188, 0, 0, 0]
85 / 2 = [94, 0, 0, 0]
85 / 3 = [62, 0, 0, 0]
85 / 4 = [47, 0, 0, 0]
90 / 1 = [177, 0, 0, 0]
90 / 2 = [88, 0, 0, 0]
90 / 3 = [59, 0, 0, 0]
90 / 4 = [44, 0, 0, 0]
95 / 1 = [168, 0, 0, 0]
95 / 2 = [84, 0, 0, 0]
95 / 3 = [56, 0, 0, 0]
95 / 4 = [42, 0, 0, 0]

However, if I change BIG_NUMBER to a bigger number (160_000 in this case):

Output:
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
65 / 1 = [1, 0, 1, 0]
65 / 2 = [1, 0, 1, 0]
65 / 3 = [1, 0, 1, 0]
65 / 4 = [1, 0, 1, 0]
70 / 1 = [1, 0, 1, 0]
70 / 2 = [1, 0, 1, 0]
70 / 3 = [1, 0, 1, 0]
70 / 4 = [1, 0, 1, 0]
5 / 1 = [1, 0, 1, 0]
5 / 2 = [1, 0, 1, 0]
5 / 3 = [1, 0, 1, 0]
5 / 4 = [1, 0, 1, 0]
80 / 1 = [1, 0, 1, 0]
80 / 2 = [1, 0, 1, 0]
80 / 3 = [1, 0, 1, 0]
80 / 4 = [1, 0, 1, 0]
85 / 1 = [1, 0, 1, 0]
85 / 2 = [1, 0, 1, 0]
85 / 3 = [1, 0, 1, 0]
85 / 4 = [1, 0, 1, 0]
0 / 1 = [1, 0, 1, 0]
0 / 2 = [1, 0, 1, 0]
0 / 3 = [1, 0, 1, 0]
0 / 4 = [1, 0, 1, 0]
95 / 1 = [1, 0, 1, 0]
95 / 2 = [1, 0, 1, 0]
95 / 3 = [1, 0, 1, 0]
95 / 4 = [1, 0, 1, 0]

Also interesting is how it's even breaking the printing of x, which is a regular u16.

It's not merely because the constant is larger than can be expressed in a u16, it has to do with the size of the resulting operation, I think. This operation does work with a much larger constant, but because of the larger sizes of the variables the result is lower and works. Something is overflowing, it seems.

@kolbma
Copy link

kolbma commented Feb 27, 2021

Yes, it is a problem of handling >16bit data for the architecture.
The instructions need 8-bit or some support 16-bit, the rest need to be handled by code from the compiler.
So I don't know the format of this data-layout-spec, I couldn't find much info about it some time ago, but the only possibility is to use 8bit "fields" for data to handle bigger ones.

I've really strange results here when using u32 data just in a calculation and doing some pin-pong with the bits on output pins.
Next to this a LCD is displaying strange output. But the display code is completely unrelated to the u32-calculation. I also use safe-code myself. So the only possibility looks like the wrong memory addresses get overwritten by the u32-stuff.
The irony is, rust wants to prevent this.
This would also explain your observation with the size of numbers. Numbers using the first 16-bits are ok, but onwards the data might be put in outland and is not handled correctly.
Do you have checked multiplication or addition instructions going through the 16-bit border?
You could isolate if the size of data is a problem or just the division-code doing something unsafe wrong.

@kolbma
Copy link

kolbma commented Feb 27, 2021

I've replaced my rust-core division with https://docs.rs/num-integer/0.1.44/num_integer/fn.div_floor.html

num-integer = { version = "0.1", default-features = false }
use num_integer::{div_floor, Integer};

let next_nr_x: u16 = nr_x.div_floor(&10);

Not sure if this works for all data sizes, but in my case the memory corruption goes away with this more or less drop-in replacement.

Edit: Ok my corruption is not fixed by this. The cause is when I change my let mut a: [u8; 5] = [b'1', b'2', b'3', b'4', b'5']; with a[1] = b'1'; After the change it is garbled. Seems to exist another problem.

Edit2: The index to access the array element has been the standard usize. After using u8 in loop and casting it to usize in the index it works again:

for idx in 0u8..(a.len() as u8) {
    a[idx as usize] = b'1';
}

Not sure what is going on here in Rust, but it is ugly.

@davystrong
Copy link

Also facing this issue.

@Patryk27
Copy link
Contributor

Patryk27 commented Oct 31, 2021

Edit: this is a wrong lead; for the correct workaround, please see: #82242 (comment).


Seems like it's due to fat LTO -- at least in my case using:

# Cargo.toml

[profile.release]
panic = "abort"
codegen-units = 1
opt-level = "z"
lto = false # or "thin"

... fixes the issue for u32, i32, u64 & i64 (128-bit numbers still seem to crash tho').

@kolbma
Copy link

kolbma commented Dec 17, 2021

@Patryk27 Where is your latest comment I got sent by GH email with the links to https://reviews.llvm.org/D114611#3178558?

Just some not bug-related question... what might be a good starting point to understand/learn how Rust -> LLVM -> Machine code works?
How would you start to debug such bugs and find the cause...
Is there a Must-Read-Book or kind-of I should start with or are there some nice introduction links?
Thx.

@Patryk27
Copy link
Contributor

@kolbma I wrote a comment about how with the https://reviews.llvm.org/D114611 patch applied, the AVR backend miscompiles __udivmodsi4, leading to stack corruption:

image

(those st + std refer to uninitialized Z, which effectively scrambles the memory and destroys the stack frame; then, when the function finishes working, ret jumps back to 0x00 or whatever address happened to be on the stack.)

... but then I realized that it's probably an oversight in that particular patch of mine (as observed by https://reviews.llvm.org/D114611#3178558) rather than a general bug in the backend, so I figured it's better to delete the comment here and focus on that LLVM's merge request 🙂

what might be a good starting point to understand/learn how Rust -> LLVM -> Machine code works?

I've just downloaded LLVM and started fiddling around it; still don't understand ~90% of the AVR's codebase, but commenting stuff out and seeing which tests break is a nice way to learn how various pieces of code come together.

How would you start to debug such bugs and find the cause...

In this particular case I've cross-compiled the Rust code for AVR and launched it under simavr -g + gdb, which allowed me to see the assembly and investigate how it works; with a debugger at hand, finding out when the stack frame got corrupted was relatively easy.

@Patryk27
Copy link
Contributor

Patryk27 commented Dec 19, 2021

I think I got it!

tl;dr: for a workaround, drop this to .cargo/config:

[unstable]
build-std = ["core"]
build-std-features = ["compiler-builtins-mangled-names"]

(128-bit operations will fail to link, but 8/16/32/64 signed/unsigned should work.)

Explanation:

Since AVR doesn't have a native instruction for division, when the LLVM AVR backend sees a division operation:

%quot = sdiv i16 %a, %b

... it expands it to a call:

call __udivmodsi4

This __udivmodsi4 (and a few other, similar functions) is expected to look more or less like this:

fn __udivmodsi4(a: u32, b: u32) -> (u32, u32) {
  /* ... */
}

... and it's expected to be linked from avr-gcc runtime library.

But, what apparently goes on, is that Rust's https://github.com/rust-lang/compiler-builtins, that also happens to provide a very similar function, takes precedence:

fn __udivmodsi4(n: u32, d: u32, rem: Option<&mut u32>) -> u32 {
  /* ... */
}

So LLVM AVR's backend is generating code that expects to be linked with avr-gcc's __udivmodsi4, but in reality it's getting the compiler-builtin's one, which has entirely different signature and just happens to be sometimes working, if the registers & moon & planets align correctly.

Adding build-std-features = ["compiler-builtins-mangled-names"] help to avoid this issue, because it causes Rust's __udivmodsi4 to be compiled under a different name, making it possible for avr-gcc to link its own __udivmodsi4 (in the past this was solved by adding no-compiler-rt: true to the target.json file, but that flag has been removed now).

I think the proper fix would be - if license allows - to copy avr-gcc's runtime files into compiler-builtins and then make those runtime files take precedence over Rust's functions; seems like that's how it works for ARM, at least.

cc @dylanmckay 🙂

Rahix added a commit to Rahix/avr-hal-template that referenced this issue Jan 28, 2022
This is a workaround to ensure intrinsics get linked in from avr-libc
instead of Rust's compiler-builtins.  The problem is that the versions
from compiler-builtins have slightly different calling convention and as
such cannot actually be substituted for the versions from avr-libc right
now.

This should fix a lot of the observed problems with 32-bit arithmetic.

Ref: rust-lang/rust#82242 (comment)
Rahix added a commit to Rahix/avr-hal that referenced this issue Jan 28, 2022
This is a workaround to ensure intrinsics get linked in from avr-libc
instead of Rust's compiler-builtins.  The problem is that the versions
from compiler-builtins have slightly different calling convention and as
such cannot actually be substituted for the versions from avr-libc right
now.

This should fix a lot of the observed problems with 32-bit arithmetic.

Ref: rust-lang/rust#82242 (comment)
Patryk27 added a commit to Patryk27/compiler-builtins that referenced this issue May 15, 2022
For division and modulo, AVR uses a custom calling convention that does
not match compiler_builtins' expectations, leading to non-working code¹.

Ideally we'd just use hand-written naked functions (as, afair, ARM
does), but that's a lot of code to port², so hopefully we'll be able to
do it gradually later.

For the time being, I'd suggest not compiling problematic functions for
AVR target - this causes avr-gcc (which is a mandatory part of Rust+AVR
toolchain anyway) to link hand-written assembly from libgcc, which is
confirmed to work.

I've tested the code locally on simavr and the patch seems to be working
correctly :-)

¹ rust-lang/rust#82242,
  rust-lang/rust#83281
² https://github.com/gcc-mirror/gcc/blob/31048012db98f5ec9c2ba537bfd850374bdd771f/libgcc/config/avr/lib1funcs.S

Closes rust-lang/rust#82242
Closes rust-lang/rust#83281
Patryk27 added a commit to Patryk27/compiler-builtins that referenced this issue May 17, 2022
For division and modulo, AVR uses a custom calling convention that does
not match compiler_builtins' expectations, leading to non-working code¹.

Ideally we'd just use hand-written naked functions (as, afair, ARM
does), but that's a lot of code to port², so hopefully we'll be able to
do it gradually later.

For the time being, I'd suggest not compiling problematic functions for
AVR target - this causes avr-gcc (which is a mandatory part of Rust+AVR
toolchain anyway) to link hand-written assembly from libgcc, which is
confirmed to work.

I've tested the code locally on simavr and the patch seems to be working
correctly :-)

¹ rust-lang/rust#82242,
  rust-lang/rust#83281
² https://github.com/gcc-mirror/gcc/blob/31048012db98f5ec9c2ba537bfd850374bdd771f/libgcc/config/avr/lib1funcs.S

Closes rust-lang/rust#82242
Closes rust-lang/rust#83281
Rahix pushed a commit to Rahix/avr-hal-template that referenced this issue Jun 16, 2022
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242
Rahix added a commit to Rahix/avr-hal that referenced this issue Jun 16, 2022
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242

Suggested-by: @Patryk27
Link: Rahix/avr-hal-template#8
Rahix added a commit to Rahix/avr-hal that referenced this issue Jun 16, 2022
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242

Suggested-by: @Patryk27
Link: Rahix/avr-hal-template#8
Rahix added a commit to Rahix/avr-hal that referenced this issue Jun 16, 2022
The `compiler-builtins-mangled-names` thingie¹ is no longer needed,
since `compiler_builtins` has been improved to avoid pulling the
problematic functions:

- rust-lang/compiler-builtins#462
- rust-lang/compiler-builtins#466
- rust-lang/rust#97435

¹ rust-lang/rust#82242

Suggested-by: @Patryk27
Link: Rahix/avr-hal-template#8
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 18, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 18, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 19, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 19, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 20, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 20, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 20, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 20, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 25, 2022
gergoerdi added a commit to gergoerdi/rust-compiler-builtins that referenced this issue Dec 25, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 26, 2022
gergoerdi added a commit to gergoerdi/chirp8-avr that referenced this issue Dec 26, 2022
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.) requires-nightly This issue requires a nightly compiler in some way.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants