Code generation error on AVR using u16::to_be()

[mutantbob]

I tried this code on AVR (arduino Uno = atmega328p) with opt-level="z":

let pixels = [0xf800u16, 0x07e0, 0x001f];
let mut buf = [0u16; 3];
{
    let mut i = 0;
    for pixel in pixels.map(u16::to_be) {
        buf[i] = pixel;
        i += 1;
    }
    let _ = uwriteln!(serial, "debug buf {:?} mapped", buf);
}

I expected to see this happen:
debug buf [248, 57351, 7936] mapped

Instead, this happened:
debug buf [0, 57568, 7967] mapped

An alternate code that emits the correct output is

    {
        for (i, pixel) in pixels.iter().enumerate() {
            buf[i] = pixel.to_be();
        }
        let _ = uwriteln!(serial, "debug buf {:?} enumerate", buf);
    }

This compiler error is corrupting the pixel stream transmitted via SPI to an ST7789 display from an Arduino Uno. What appears to be happening is that instead of the bytes being swapped, the LSB is being copied to the MSB. I do not have the expertise to decompile and examine the assembly code.

Meta

rustc --version --verbose:

rustc 1.62.0-nightly (88860d547 2022-05-09)
binary: rustc
commit-hash: 88860d5474a32f507dde8fba8df35fd2064f11b9
commit-date: 2022-05-09
host: x86_64-unknown-linux-gnu
release: 1.62.0-nightly
LLVM version: 14.0.1

The malfunction also occurs with cargo +nightly.

rustc +nightly --version --verbose

rustc 1.63.0-nightly (b31f9cc22 2022-06-15)
binary: rustc
commit-hash: b31f9cc22bcd720b37ddf927afe378108a5b9a54
commit-date: 2022-06-15
host: x86_64-unknown-linux-gnu
release: 1.63.0-nightly
LLVM version: 14.0.5

The full code can be cloned from https://github.com/mutantbob/rust-avr-code-generation-bug

[jamesmunns]

Since I looked it up, here's the expected/actual in hex, which makes the bug more clear:

input:

[0xf800, 0x07e0, 0x001f]

expected:

debug buf [0x00f8, 0xe007, 0x1f00] mapped

actual:

debug buf [0x0000, 0xe0e0, 0x1f1f] mapped

[Patryk27]

Hi,

Could you please check with opt-level = 1? It feels like a bug nonetheless, but I'm curious about the result 🙂

[Patryk27]

Also:
@rustbot claim

[Patryk27]

I'm analyzing the codegen bug - simplest reproduction so far:

#![no_std]
#![no_main]

use panic_halt as _;

#[arduino_hal::entry]
fn main() -> ! {
    let pixels = [0];

    for pixel in pixels.map(|n| n + 1) { // works with `.into_iter().map()`
        print(pixel);
    }

    loop {
        //
    }
}

#[inline(never)]
fn print(pixel: u16) {
    let dp = unsafe { arduino_hal::Peripherals::steal() };
    let pins = arduino_hal::pins!(dp);
    let mut serial = arduino_hal::default_serial!(dp, pins, 57600);
    let _ = ufmt::uwriteln!(serial, "{}", pixel);
}

[Patryk27]

Got it!

The simplest reproduction:

#![no_std]
#![no_main]
#![feature(bench_black_box)]

use core::hint::black_box;
use panic_halt as _;

#[arduino_hal::entry]
fn main() -> ! {
    if let Some(value) = next() {
        print(value);
    }

    loop {
        //
    }
}

#[inline(never)]
fn next() -> Option<u16> { // this corresponds to `array.into_iter().next()` from the original code
                           // (note that the author doesn't call those functions by hand, but that's
                           //  how the loop gets desugared)
    black_box(Some(12345))
}

#[inline(never)]
fn print(n: u16) {
    let dp = unsafe { arduino_hal::Peripherals::steal() };
    let pins = arduino_hal::pins!(dp);
    let mut serial = arduino_hal::default_serial!(dp, pins, 57600);
    let _ = ufmt::uwriteln!(serial, "{}", n);
}

This prints 14649 (0x3939 in hex) instead of 12345 (0x3039 in hex), because the generated binary accidentally overwrites the high-order byte with the low-order one:

After calling next() (call 0xa6 on the screenshot), the returned number - 12345 (0x3039 in hex) - gets stored into r23:r24 (you can see that r23=0x39, r24=0x30, and AVR being little-endian means the number is reconstructed from r24 first, r23 second).

print() expects its n: u16 parameter to be stored in r24:r25, so right before calling print(), the codegen has a simple job: move r23:r24 into r24:r25; and that's where the bug comes into play.

What the code generator says is:

; (so r23:r24 is the input number; r24:r25 is where we want the number to be.)

mov r24, r23
mov r25, r24

... which always sets r24 and r25 to the same value (r23) - what it should generate instead is:

mov r25, r24
mov r24, r23

Overall, that's an omission on the LLVM's side (when moving register pairs, it should consider whether the registers are overlapping or not) - I'll try to prepare a patch 🙂

Edit: https://reviews.llvm.org/D128588.