u128 miscompilation in 1.63.0.2

[ollpu]

The following code

#[derive(Debug)]
struct U32Pair {
    a: u32,
    b: u32,
}

#[inline(never)]
fn u128_test(p1: U32Pair) {
    let x = dbg!(p1.a) as u128 * 10;
    dbg!(x);
    let y = x + 1;
    dbg!(y);
}

fn main() {
    u128_test(U32Pair { a: 1, b: 0 });
}

prints (on -C opt-level=z)

[src/main.rs:50] p1.a = 1
[src/main.rs:51] x = 0
[src/main.rs:53] y = 10000000000000000001

The test is reduced from std::thread::sleep which currently gets stuck sleeping forever. It's pretty particular about the details, otherwise it might work correctly or cause panics in the format code.

I've also added this to https://github.com/ollpu/esp-rustc-abi-bug

[MabezDev]

Thanks for the test case!

I'm really struggling with this one, I'm pretty sure anything that is printed is wrong.

For instance, if you add a couple of asserts to that test case:

let x = dbg!(p1.a) as u128 * 10;
dbg!(x);
assert_eq!(x, 10);

You'll find that the actual values there are correct. I have also confirmed this by printing the numbers a byte at a time and reconstructing the number manually, which indicates an issue with printing.

I imagine some part of the fmt machinery is exercising a buggy part of the ABI, but I haven't managed to track that down yet.

What this doesn't explain is why sleep is not working for you 🤔.

[ollpu]

Good point, I may not be triggering any incorrect behavior other than the prints.

Now I tried modifying std like this:

    #[inline(never)]
    pub fn observe_u32(val: u32) {
        println!("{val}");
    }

    #[cfg(target_os = "espidf")]
    pub fn sleep(dur: Duration) {
        let mut micros = dur.as_micros();
        unsafe {
            while micros > 0 {
                let st = if micros > u32::MAX as u128 { u32::MAX } else { micros as u32 };
                Self::observe_u32(st);
                libc::usleep(st);

                micros -= st as u128;
            }
        }
    }

This keeps printing 4294967295 indefinitely, but this doesn't happen if I copy the function into my own code as is. Different compiler flags for std?

[MabezDev]

Different compiler flags for std?

I don't think so, because we use the build-std feature of cargo, it should build core/std with the same args as the project.

[MabezDev]

Hmm, perhaps this is related to the depth of the call stack, just copying some offending functions to the demo application doesn't cause the issue 🤔

[ollpu]

This might be completely unrelated to this bug, but something fishy is still going on with the calling ABI. This is the part of as_millis() that calls __multi to perform a 128-bit multiplication, seconds by 1000000.

	l32i.n	a10, a2, 0
	l32i.n	a11, a2, 4
	mov.n	a8, a1
	movi.n	a15, 0
	s32i	a15, a1, 88
	s32i.n	a15, a8, 4
	s32i.n	a15, a8, 0
	l32r	a14, .LCPI3207_0  # 1000000
	l32r	a8, .LCPI3207_1   # __multi
	mov.n	a12, a15
	mov.n	a13, a15
	callx8	a8
	l32i	a8, a1, 88
	s32i	a13, a1, 92
	s32i	a12, a1, 96
	s32i	a11, a1, 100
	s32i	a10, a1, 104

So to my reading, the first parameter is correctly put in a10:a11 (u64 parameter) and a12:a13 (zero), but the second parameter is incorrectly split into a14 (constant), a15 (zero), and stack at [a8+0]:[a8+4].

Could it be that the LLVM ABI implementation you ported is also buggy? I'd assume they are still compatible with each other, which is why this might not be the cause of this particular bug.

Not sure how I could see the assembly of the __multi intrinsic.

[simonchatts]

Just to say, I have also observed this, with 1.63.0.2, only on xtensa (almost the same app works fine on an ESP32-C3).

Duration::as_micros() fails (under the guise of thread::sleep() triggering watchdog), and Duration::as_millis() in regular application code gives totally bogus values.

[MabezDev]

Hey folks,

I've spent the last week debugging the current ABI, reading various Xtensa ISA documents, as well as looking into the Xtensa LLVM backend and how it handles parts of the ABI in codegen.

I've just pushed esp-1.63.0.3 branch which I believe now solves all the ABI issues. I would appreciate it if you could build that branch and test it out before I release this :).

FYI on my journey, I found another bug related to some 128bit integer math, which I have reported internally. Hopefully, that should get sorted soon too :).

Thank you for your patience!

[ollpu]

Can confirm that the original test now works correctly, as does std::thread::sleep. Thank you!

[simonchatts]

Thanks for all the work @MabezDev! I can confirm the 1.63.0.3 branch solves both my u128 issues.

[PTD110]

rust-esp32-std-demo seems to run fine now - no longer backtraces after std::thread::sleep. Thank you @MabezDev for tackling this!

[Zagitta]

It should be noted this also manifests itself as UART Rx::read_bytes_blocking not actually blocking 😅

[thefill]

@MabezDev any plans to release this?

[MabezDev]

Due to the release of 1.64.0.0 being so soon, we've released that instead of 1.63.0.3. The branch is available here: https://github.com/esp-rs/rust. Builds will be available soon, but slower than usual as we have to rebuild LLVM too.

[thefill]

Due to the release of 1.64.0.0 being so soon, we've released that instead of 1.63.0.3. The branch is available here: https://github.com/esp-rs/rust. Builds will be available soon, but slower than usual as we have to rebuild LLVM too.

legend, thx for clarification