From da44e3fdceb46b8d53b2a6e93bca3b80d4243a17 Mon Sep 17 00:00:00 2001 From: Julian Frimmel Date: Mon, 14 Oct 2024 23:06:32 +0200 Subject: [PATCH 1/6] Start test case for `rjmp` regression test 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]: https://github.com/rust-lang/rust/issues/129301#issuecomment-2301399472 --- tests/assembly/avr-rjmp-offsets.rs | 62 ++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 tests/assembly/avr-rjmp-offsets.rs diff --git a/tests/assembly/avr-rjmp-offsets.rs b/tests/assembly/avr-rjmp-offsets.rs new file mode 100644 index 0000000000000..fafa2b43138da --- /dev/null +++ b/tests/assembly/avr-rjmp-offsets.rs @@ -0,0 +1,62 @@ +//@ compile-flags: -Copt-level=s --target=avr-unknown-gnu-atmega328 -C panic=abort +//@ needs-llvm-components: avr +//@ assembly-output: emit-asm + +#![feature( + no_core, + lang_items, + intrinsics, + rustc_attrs, + arbitrary_self_types, + asm_experimental_arch +)] +#![crate_type = "rlib"] +#![no_core] + +#[rustc_builtin_macro] +macro_rules! asm { + () => {}; +} + +use minicore::ptr; + +// CHECK-LABEL: pin_toggling +#[no_mangle] +pub fn pin_toggling() { + let port_b = 0x25 as *mut u8; // the I/O-address of PORTB + loop { + unsafe { ptr::write_volatile(port_b, 1) }; + delay(500_0000); + unsafe { ptr::write_volatile(port_b, 2) }; + delay(500_0000); + } +} + +#[inline(never)] +fn delay(_: u32) { + unsafe { asm!("nop") }; +} + +// FIXME: replace with proper minicore once available (#130693) +mod minicore { + #[lang = "sized"] + pub trait Sized {} + + #[lang = "copy"] + pub trait Copy {} + impl Copy for u32 {} + impl Copy for &u32 {} + impl Copy for *mut T {} + + pub mod ptr { + #[inline] + #[rustc_diagnostic_item = "ptr_write_volatile"] + pub unsafe fn write_volatile(dst: *mut T, src: T) { + extern "rust-intrinsic" { + #[rustc_nounwind] + pub fn volatile_store(dst: *mut T, val: T); + } + unsafe { volatile_store(dst, src) }; + } + } +} From 652ba6699c30f036be07fead0df63a1e5f5ddbf8 Mon Sep 17 00:00:00 2001 From: Julian Frimmel Date: Tue, 15 Oct 2024 12:23:35 +0200 Subject: [PATCH 2/6] Add check-annotations ensuring correct label The issue was, that the disassembled label was placed one instruction further down than expected. Therefore the test annotations check, that the label is placed above the loop-contents (writing the one value, then writing the other one). --- tests/assembly/avr-rjmp-offsets.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/assembly/avr-rjmp-offsets.rs b/tests/assembly/avr-rjmp-offsets.rs index fafa2b43138da..bf43e016f92b7 100644 --- a/tests/assembly/avr-rjmp-offsets.rs +++ b/tests/assembly/avr-rjmp-offsets.rs @@ -21,6 +21,12 @@ macro_rules! asm { use minicore::ptr; // CHECK-LABEL: pin_toggling +// CHECK: .LBB0_1: +// CHECK-NEXT: out 5, r17 +// CHECK-NEXT: call delay +// CHECK-NEXT: out 5, r16 +// CHECK-NEXT: call delay +// CHECK-NEXT: rjmp .LBB0_1 #[no_mangle] pub fn pin_toggling() { let port_b = 0x25 as *mut u8; // the I/O-address of PORTB @@ -33,6 +39,7 @@ pub fn pin_toggling() { } #[inline(never)] +#[no_mangle] fn delay(_: u32) { unsafe { asm!("nop") }; } From db6c736da3df6ab7be13c90ac0c13c21b2ba6920 Mon Sep 17 00:00:00 2001 From: Julian Frimmel Date: Tue, 15 Oct 2024 12:30:55 +0200 Subject: [PATCH 3/6] Allow the compiler to select any register --- tests/assembly/avr-rjmp-offsets.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/assembly/avr-rjmp-offsets.rs b/tests/assembly/avr-rjmp-offsets.rs index bf43e016f92b7..0acf54fada46a 100644 --- a/tests/assembly/avr-rjmp-offsets.rs +++ b/tests/assembly/avr-rjmp-offsets.rs @@ -21,10 +21,12 @@ macro_rules! asm { use minicore::ptr; // CHECK-LABEL: pin_toggling +// CHECK: ldi [[REG_1:r[0-9]+]], 1 +// CHECK: ldi [[REG_2:r[0-9]+]], 2 // CHECK: .LBB0_1: -// CHECK-NEXT: out 5, r17 +// CHECK-NEXT: out 5, [[REG_1]] // CHECK-NEXT: call delay -// CHECK-NEXT: out 5, r16 +// CHECK-NEXT: out 5, [[REG_2]] // CHECK-NEXT: call delay // CHECK-NEXT: rjmp .LBB0_1 #[no_mangle] From ab008414d4b4919c1344ef0d542fc9b0d354a505 Mon Sep 17 00:00:00 2001 From: Julian Frimmel Date: Tue, 15 Oct 2024 20:46:23 +0200 Subject: [PATCH 4/6] Convert to a `rmake`-test Since the `tests/assembly` use `emit=asm`, the issue is not observable as reported in the linked issue. Therefore the existing test case is converted and a simple `rmake`-test is added. The test only checks, if the correct `rjmp`-offset is used. --- .../avr-rjmp-offset}/avr-rjmp-offsets.rs | 35 +++++++------------ tests/run-make/avr-rjmp-offset/rmake.rs | 28 +++++++++++++++ 2 files changed, 40 insertions(+), 23 deletions(-) rename tests/{assembly => run-make/avr-rjmp-offset}/avr-rjmp-offsets.rs (63%) create mode 100644 tests/run-make/avr-rjmp-offset/rmake.rs diff --git a/tests/assembly/avr-rjmp-offsets.rs b/tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs similarity index 63% rename from tests/assembly/avr-rjmp-offsets.rs rename to tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs index 0acf54fada46a..d0e5ef1997380 100644 --- a/tests/assembly/avr-rjmp-offsets.rs +++ b/tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs @@ -1,17 +1,11 @@ -//@ compile-flags: -Copt-level=s --target=avr-unknown-gnu-atmega328 -C panic=abort -//@ needs-llvm-components: avr -//@ assembly-output: emit-asm - -#![feature( - no_core, - lang_items, - intrinsics, - rustc_attrs, - arbitrary_self_types, - asm_experimental_arch -)] -#![crate_type = "rlib"] +//! This test case is a `#![no_core]`-version of the MVCE presented in #129301. +//! +//! The function [`delay()`] is minimized and does not actually contain a loop +//! in order to remove the need for additional lang items. +#![feature(no_core, lang_items, intrinsics, rustc_attrs, asm_experimental_arch)] #![no_core] +#![no_main] +#![allow(internal_features)] #[rustc_builtin_macro] macro_rules! asm { @@ -20,18 +14,13 @@ macro_rules! asm { use minicore::ptr; -// CHECK-LABEL: pin_toggling -// CHECK: ldi [[REG_1:r[0-9]+]], 1 -// CHECK: ldi [[REG_2:r[0-9]+]], 2 -// CHECK: .LBB0_1: -// CHECK-NEXT: out 5, [[REG_1]] -// CHECK-NEXT: call delay -// CHECK-NEXT: out 5, [[REG_2]] -// CHECK-NEXT: call delay -// CHECK-NEXT: rjmp .LBB0_1 #[no_mangle] -pub fn pin_toggling() { +pub fn main() -> ! { let port_b = 0x25 as *mut u8; // the I/O-address of PORTB + + // a simple loop with some trivial instructions within. This loop label has + // to be placed correctly before the `ptr::write_volatile()` (some LLVM ver- + // sions did place it after the first loop instruction, causing unsoundness) loop { unsafe { ptr::write_volatile(port_b, 1) }; delay(500_0000); diff --git a/tests/run-make/avr-rjmp-offset/rmake.rs b/tests/run-make/avr-rjmp-offset/rmake.rs new file mode 100644 index 0000000000000..666aa41ef4176 --- /dev/null +++ b/tests/run-make/avr-rjmp-offset/rmake.rs @@ -0,0 +1,28 @@ +//@ needs-llvm-components: avr +//! Regression test for #129301/llvm-project#106722 within `rustc`. +//! +//! Some LLVM-versions had wrong offsets in the local labels, causing the first +//! loop instruction to be missed. This test therefore contains a simple loop +//! with trivial instructions in it, to see, where the label is placed. +//! +//! This must be a `rmake`-test and cannot be a `tests/assembly`-test, since the +//! wrong output is only produced with direct assembly generation, but not when +//! "emit-asm" is used, as described in the issue description of #129301: +//! https://github.com/rust-lang/rust/issues/129301#issue-2475070770 +use run_make_support::{llvm_objdump, rustc}; + +fn main() { + rustc() + .input("avr-rjmp-offsets.rs") + .opt_level("s") + .panic("abort") + .target("avr-unknown-gnu-atmega328") + .output("compiled") + .run(); + + llvm_objdump() + .disassemble() + .input("compiled") + .run() + .assert_stdout_contains_regex(r"rjmp.*\.-14"); +} From bb8db13892ced757faf1c0206c70f313cd4b23c0 Mon Sep 17 00:00:00 2001 From: Julian Frimmel Date: Tue, 15 Oct 2024 21:04:06 +0200 Subject: [PATCH 5/6] Simplify test and make it more reliable The new `rmake`-content asserts the exact assembly sequence for the loop preventing false-negatives if some instructions would change and thus the label offset might need to change. --- .../avr-rjmp-offset/avr-rjmp-offsets.rs | 19 ++--------- tests/run-make/avr-rjmp-offset/rmake.rs | 33 ++++++++++++++++--- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs b/tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs index d0e5ef1997380..2f97fc1ed95ab 100644 --- a/tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs +++ b/tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs @@ -1,17 +1,12 @@ //! This test case is a `#![no_core]`-version of the MVCE presented in #129301. //! -//! The function [`delay()`] is minimized and does not actually contain a loop -//! in order to remove the need for additional lang items. -#![feature(no_core, lang_items, intrinsics, rustc_attrs, asm_experimental_arch)] +//! The function [`delay()`] is removed, as it is not necessary to trigger the +//! wrong behavior and would require some additional lang items. +#![feature(no_core, lang_items, intrinsics, rustc_attrs)] #![no_core] #![no_main] #![allow(internal_features)] -#[rustc_builtin_macro] -macro_rules! asm { - () => {}; -} - use minicore::ptr; #[no_mangle] @@ -23,18 +18,10 @@ pub fn main() -> ! { // sions did place it after the first loop instruction, causing unsoundness) loop { unsafe { ptr::write_volatile(port_b, 1) }; - delay(500_0000); unsafe { ptr::write_volatile(port_b, 2) }; - delay(500_0000); } } -#[inline(never)] -#[no_mangle] -fn delay(_: u32) { - unsafe { asm!("nop") }; -} - // FIXME: replace with proper minicore once available (#130693) mod minicore { #[lang = "sized"] diff --git a/tests/run-make/avr-rjmp-offset/rmake.rs b/tests/run-make/avr-rjmp-offset/rmake.rs index 666aa41ef4176..28018f59c5bb6 100644 --- a/tests/run-make/avr-rjmp-offset/rmake.rs +++ b/tests/run-make/avr-rjmp-offset/rmake.rs @@ -20,9 +20,32 @@ fn main() { .output("compiled") .run(); - llvm_objdump() - .disassemble() - .input("compiled") - .run() - .assert_stdout_contains_regex(r"rjmp.*\.-14"); + let disassembly = llvm_objdump().disassemble().input("compiled").run().stdout_utf8(); + + // search for the following instruction sequence: + // ```disassembly + // 00000080
: + // 80: 81 e0 ldi r24, 0x1 + // 82: 92 e0 ldi r25, 0x2 + // 84: 85 b9 out 0x5, r24 + // 86: 95 b9 out 0x5, r25 + // 88: fd cf rjmp .-6 + // ``` + // This matches on all instructions, since the size of the instructions be- + // fore the relative jump has an impact on the label offset. Old versions + // of the Rust compiler did produce a label `rjmp .-4` (misses the first + // instruction in the loop). + disassembly + .trim() + .lines() + .skip_while(|&line| !line.contains("
")) + .inspect(|line| println!("{line}")) + .skip(1) + .zip(["ldi\t", "ldi\t", "out\t", "out\t", "rjmp\t.-6"]) + .for_each(|(line, expected_instruction)| { + assert!( + line.contains(expected_instruction), + "expected instruction `{expected_instruction}`, got `{line}`" + ); + }); } From a35ed2f9eb52176b14e27f0f39229015cf035a30 Mon Sep 17 00:00:00 2001 From: Julian Frimmel Date: Tue, 15 Oct 2024 23:07:00 +0200 Subject: [PATCH 6/6] Use `rust-lld` instead of `avr-gcc` as the linker This fixes the [build error] caused by the `avr-gcc` (used as linker) not being available in the Rust CI. This is a viable solution, which shows the wrong/right behavior and, since no functions from `libgcc` are called, does not produce errors. This was discussed [here]. Another small problem is, that `lld` doesn't link the correct startup-code by default. This is not a problem for this test (since it does not actually use anything the startup code is needed for (no variables, no stack, no interrupts)), but this causes the `main`-function to be removed by the default flag `--gc-sections`. Therefore the `rmake`-driver also adds the linker flag `--entry=main` to mark the `main`-function as the entry point and thus preventing it from getting removed. The code would work on a real AVR device. [build error]: https://github.com/rust-lang/rust/pull/131755#issuecomment-2415127952 [here]: https://github.com/rust-lang/rust/pull/131755#issuecomment-2416469675 --- tests/run-make/avr-rjmp-offset/rmake.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/run-make/avr-rjmp-offset/rmake.rs b/tests/run-make/avr-rjmp-offset/rmake.rs index 28018f59c5bb6..89cbca309be09 100644 --- a/tests/run-make/avr-rjmp-offset/rmake.rs +++ b/tests/run-make/avr-rjmp-offset/rmake.rs @@ -1,4 +1,5 @@ //@ needs-llvm-components: avr +//@ needs-rust-lld //! Regression test for #129301/llvm-project#106722 within `rustc`. //! //! Some LLVM-versions had wrong offsets in the local labels, causing the first @@ -17,6 +18,13 @@ fn main() { .opt_level("s") .panic("abort") .target("avr-unknown-gnu-atmega328") + // normally one links with `avr-gcc`, but this is not available in CI, + // hence this test diverges from the default behavior to enable linking + // at all, which is necessary for the test (to resolve the labels). To + // not depend on a special linker script, the main-function is marked as + // the entry function, causing the linker to not remove it. + .linker("rust-lld") + .link_arg("--entry=main") .output("compiled") .run(); @@ -35,6 +43,7 @@ fn main() { // fore the relative jump has an impact on the label offset. Old versions // of the Rust compiler did produce a label `rjmp .-4` (misses the first // instruction in the loop). + assert!(disassembly.contains("
"), "no main function in output"); disassembly .trim() .lines()