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

Incorrect relative jump (rjmp) code generation in latest nightly release for AVR toolchain #129301

Closed
VivekYadav7272 opened this issue Aug 20, 2024 · 3 comments · Fixed by #131755
Assignees
Labels
A-codegen Area: Code generation A-inline-assembly Area: Inline assembly (`asm!(…)`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. F-asm `#![feature(asm)]` (not `llvm_asm`) I-miscompile Issue: Correct Rust code lowers to incorrect machine code O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@VivekYadav7272
Copy link

VivekYadav7272 commented Aug 20, 2024

I've already asked this question on stack exchange where you can see more details.

I tried the following code:

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

use core::{arch::asm, hint::black_box, panic::PanicInfo};

use avrd::atmega328p;

#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
    loop {}
}

#[inline(never)]
fn delay(x: u32) {
    for _ in 0..x {
        unsafe {
            asm!("nop");
        }
    }
}

unsafe fn write_reg(reg: *mut u8, val: u8, mask: u8) {
    let reg_val = reg.read_volatile();
    reg.write_volatile((reg_val & !mask) | (val & mask));
}

#[no_mangle]
extern "C" fn main() -> ! {
    const LED_BUILTIN: u8 = 5;
    unsafe {
        let portB_data_direction = atmega328p::DDRB;
        // set it to output mode
        write_reg(portB_data_direction, 1 << LED_BUILTIN, 1 << LED_BUILTIN);
        let portB = atmega328p::PORTB;
        // switch it on, hopefully..
        loop {
            write_reg(portB, 1 << LED_BUILTIN, 1 << LED_BUILTIN);
            delay(500_0000);
            write_reg(portB, 0, 1 << LED_BUILTIN);
            delay(500_0000);
        }
    }
}

The RUSTFLAGS="--emit asm" output is also correct, but the machine code generated is faulty. For now I've switched to nightly-2023-12-11, which seems to work just fine.

For the disassembly of both the --emit asm and the final binary, you can refer to the stack exchange post.
Here's a minimal example that doesn't work on nightly (but works on nightly-2023-12-11):

#![no_std]
#![no_main]

use core::{hint::black_box, panic::PanicInfo};

use avrd::atmega328p;

#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
    loop {}
}

#[no_mangle]
extern "C" fn main() -> ! {
    const LED_BUILTIN: u8 = 5;
    unsafe {
        let portB_data_direction = atmega328p::DDRB;
        // set it to output mode
        *portB_data_direction |= 1 << LED_BUILTIN;
        let portB = atmega328p::PORTB;

        loop {
            *portB ^= 1 << LED_BUILTIN;
            for i in 0..500_000 {
                black_box(i);
            }
            *portB ^= 1 << LED_BUILTIN;
            for i in 0..500_000 {
                black_box(i);
            }
        }
    }
}
@VivekYadav7272 VivekYadav7272 added the C-bug Category: This is a bug. label Aug 20, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 20, 2024
@jieyouxu jieyouxu added A-codegen Area: Code generation O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) labels Aug 20, 2024
@saethlin saethlin added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 21, 2024
@edgar-bonet
Copy link

The TLDR of the referenced discussion is that a Rust loop that looks in essence like this:

let portB = atmega328p::PORTB;  // A microcontroller's I/O port
loop {
    portB.write_volatile(portB.read_volatile() | (1<<5));   // set bit 5 of PORTB
    delay(500_0000);
    portB.write_volatile(portB.read_volatile() & ~(1<<5));  // clear bit 5 of PORTB
    delay(500_0000);
}

gets translated by --emit asm into this:

loop:
    sbi  PORTB, 5  ; set bit 5 of PORTB
    call delay
    cbi  PORTB, 5  ; clear bit 5 of PORTB
    call delay
    rjmp loop      ; relative jump to the start of the loop

which is correct. However, direct translation to machine code yields something that disassembles into this:

    sbi  PORTB, 5  ; set bit 5 of PORTB
loop:              ; <- wrong place
    call delay
    cbi  PORTB, 5  ; clear bit 5 of PORTB
    call delay
    rjmp loop      ; relative jump to the wrong place

This also affects conditional branches (seen on brcs, breq) which, just like rjmp, use PC-relative addressing.

@workingjubilee workingjubilee added the I-miscompile Issue: Correct Rust code lowers to incorrect machine code label Oct 2, 2024
@Patryk27
Copy link
Contributor

This was fixed over llvm/llvm-project#106722 and the newest rustc emits correct code 🙂

@Urgau Urgau added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 14, 2024
@jfrimmel
Copy link
Contributor

I'll add a regression test.

@rustbot claim

jfrimmel added a commit to jfrimmel/rust that referenced this issue Oct 15, 2024
This commit introduces a minimal `![no_core]`-test case running on AVR,
that contains the MCWE mentioned in [129301]. The test case itself does
not have any assertions yet, but it shows the minimal set an language
items necessary within the test case.

[129301]: rust-lang#129301 (comment)
@bors bors closed this as completed in ebff167 Oct 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 18, 2024
Rollup merge of rust-lang#131755 - jfrimmel:avr-rjmp-offset-regression-test, r=jieyouxu

Regression test for AVR `rjmp` offset

This adds a regression test for rust-lang#129301 by minimizing the code in the linked issue and putting it into a `#![no_core]`-compatible format, so that it can easily be used within an `rmake`-test. This needs to be a `rmake` test (opposed to a `tests/assembly` one), since the linked issue describes, that the problem only occurs if the code is directly compiled. Note, that `lld` is used instead of `avr-gcc`; see the [comments](rust-lang#131755 (comment)) [below](rust-lang#131755 (comment)).
Closes rust-lang#129301.

To show, that the test actually catches the wrong behavior, this can be tested with a faulty rustc:
```bash
$ rustup install nightly-2024-08-19
$ rustc +nightly-2024-08-19 -C opt-level=s -C panic=abort --target avr-unknown-gnu-atmega328 -Clinker=build/x86_64-unknown-linux-gnu/ci-llvm/bin/lld -Clink-arg='--entry=main' -o compiled tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs
$ llvm-objdump -d compiled | grep '<main>' -A 6
000110b4 <main>:
   110b4: 81 e0         ldi     r24, 0x1
   110b6: 92 e0         ldi     r25, 0x2
   110b8: 85 b9         out     0x5, r24
   110ba: 95 b9         out     0x5, r25
   110bc: fe cf         rjmp    .-4
```
One can see, that the wrong label offset (`4` instead of `6`) is produced, which would trigger an assertion in the test case.

This would be a good candidate for using the `minicore` proposed in rust-lang#130693. Since this is not yet merged, there is a FIXME.

r? Patryk27
I think, you are the yet-to-be official target maintainer, hence I'll assign to you.

`@rustbot` label +O-AVR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-inline-assembly Area: Inline assembly (`asm!(…)`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. F-asm `#![feature(asm)]` (not `llvm_asm`) I-miscompile Issue: Correct Rust code lowers to incorrect machine code O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants