Regression test for AVR rjmp offset

[jfrimmel]

This adds a regression test for #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 below.
Closes #129301.

To show, that the test actually catches the wrong behavior, this can be tested with a faulty rustc:

$ 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 #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

[rustbot]

Failed to set assignee to Patryk27: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[rustbot]

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

[tgross35]

r? compiler
cc @Patryk27

[jfrimmel]

Hm, there's a problem: it works locally, but not in CI. CI reports either of two issues:

1. when trying to perform linking (the more correct approach), the CI job fails due to the linker being missing. AVR is kinda special here, because (presumably) it's the only target relying on the GCC as its linker. Is there a sane work around this for the test?
2. when not performing linking (by only building an rlib), the labe does not get resolved in CI (but on my machine?!), causing the rjmp to have an offset of 0, which renders the test unusable.

I could force -Clinker=lld, which (locally) works, but I don't know, if diverging from the suggested tool-flow in tests is good practice.
Has anyone an idea?

[jieyouxu]

What do you mean by suggested tool-flow?

[jfrimmel]

Even if not yet merged, PR #131651 documents, that the avr-gcc-linker should be used:

Compiling for this target requires avr-gcc, because a couple of intrinsics (like 32-bit multiplication) rely on libgcc and can't be provided through compiler-builtins yet; this is a limitation that we hope to lift in the future, see rust-lang/compiler-builtins#711.

I called the fact, that one should use avr-gcc as the linker (for special calling conventions and libgcc), as "suggested tool-flow". Note, that the test itself neither calls special functions nor links to libgcc in any way (thanks to no arithmetic or similar being used at runtime), so this should not matter and ldd might be a viable option.

[jieyouxu]

1. when trying to perform linking (the more correct approach), the CI job fails due to the linker being missing. AVR is kinda special here, because (presumably) it's the only target relying on the GCC as its linker. Is there a sane work around this for the test?
2. when not performing linking (by only building an rlib), the labe does not get resolved in CI (but on my machine?!), causing the rjmp to have an offset of 0, which renders the test unusable.

I could force -Clinker=lld, which (locally) works, but I don't know, if diverging from the suggested tool-flow in tests is good practice. Has anyone an idea?

... I mean, kinda, if you add a linker detection directive in compiletest that conditionally runs the test if avr-gcc is available. However, that means this test will never get exercised in CI because AFAIK the only avr target we have is exactly the one proposed in the PR you linked, which is Tier 3, and we don't test tier 3 targets in CI¹.

And if we're not testing the "standard" recommended linker, it won't reflect the actual avr environment that users usually have, right? Although if e.g. the regression can be reproduced with using a linker other than avr-gcc, such as lld or such, then I'm fine with that too.

Basically, if using a different linker will still repro the regression, then I think it's okay to use a linker that's not part of the "default" tool-flow.

Footnotes

1. If this test never gets exercised by CI, then it seems questionable to add it in the first place. If the avr target ever gets promoted to Tier 2 and does get exercised in CI, then we might even be able to ask T-infra if it's possible to incl. avr-gcc somehow. ↩

[jfrimmel]

Thank you for your feedback! Based on that I propose: we add the test with lld as the linker. While not the de-facto standard linker for that target, it clearly shows the behavior. Since no libgcc needs to be linked in, this will work for this test. It is good to have this test, so that we prevent having a compiler producing unsound code in the future.

[jieyouxu]

This seems like a reasonable compromise to me. I'll r+ when PR CI is green. Thanks.

[jfrimmel]

Thany very much for your support!
I've included the aforementioned directive, so it should be good to go (altough the PR CI won't run the test, therefore it might fail in rollup).

Again: Thank you!

[jieyouxu]

@bors r+ rollup=iffy (linker dependent)

[bors]

📌 Commit a35ed2f has been approved by jieyouxu

It is now in the queue for this repository.

[jieyouxu]

@bors r- I need to double check the rust-lld directive, seems sus (not actionable for you, waiting on me)

[jieyouxu]

I don't see why this won't run in at least one CI, but maybe T-infra will tell me a few months later 😆 Anyway
@bors r+

[bors]

📌 Commit a35ed2f has been approved by jieyouxu

It is now in the queue for this repository.

[bors]

⌛ Testing commit a35ed2f with merge 1350eea...

[jieyouxu]

@bors r- (bors???)

[jieyouxu]

@jfrimmel hi, this test seems to have spuriously failed in #131527 (comment) with a heap corruption... does that look genunine to you?

[jieyouxu]

---- [run-make] tests\run-make\avr-rjmp-offset stdout ----

error: rmake recipe failed to complete
status: exit code: 1
command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake.exe"
--- stderr -------------------------------
command failed at line 29
command failed at line 29
Command { cmd: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\stage2\\bin\\rustc.exe" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake_out" "avr-rjmp-offsets.rs" "-Copt-level=s" "-Cpanic=abort" "--target=avr-unknown-gnu-atmega328" "-Clinker=rust-lld" "-Clink-arg=--entry=main" "-o" "compiled", stdin_buf: None, stdin: None, stdout: None, stderr: None, drop_bomb: DropBomb { command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\stage2\\bin\\rustc.exe", defused: true, armed_location: Location { file: "C:\\a\\rust\\rust\\tests\\run-make\\avr-rjmp-offset\\rmake.rs", line: 16, col: 5 } }, already_executed: true }
output status: `exit code: 1`
=== STDOUT ===


=== STDERR ===
error: linking with `rust-lld` failed: exit code: 0xc0000374
error: linking with `rust-lld` failed: exit code: 0xc0000374
  |
  = note: "rust-lld" "-flavor" "gnu" "C:\\a\\_temp\\msys64\\tmp\\rustc4vG81h\\symbols.o" "compiled.avr_rjmp_offsets.ab053966543a1f9f-cgu.0.rcgu.o" "--as-needed" "-Bdynamic" "-z" "noexecstack" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-gnu\\test\\run-make\\avr-rjmp-offset\\rmake_out" "-o" "compiled" "--gc-sections" "--entry=main"

error: aborting due to 1 previous error
------------------------------------------

[jieyouxu]

Wait that looks like the linker died, maybe similar to #115985

[jfrimmel]

@jfrimmel hi, this test seems to have spuriously failed in #131527 (comment) with a heap corruption... does that look genunine to you?

Puh, I don't thibk, this is really actionable by me. I don't have the CI environment here to try to reproduce it and the linked issue looks like this is spurious behavior. Of course, the underlying issue is not introduced by this PR but rather "just" unveiled.
If necessary, I can disable the test on Windows platforms, but this would only hide the issue (but would lead to potentially less spurious fails in unrelated PRs). If I should do so, feel free to request it.

[jieyouxu]

Yeah mb, I don't think it's the test itself, it looks like an underlying bug with rust-lld.

[ehuss]

This test seems to be failing somewhat often:

• 2024-11-25 Hang: Update cargo and books #133465 (comment)
• 2024-11-20 Error: #124141 preliminaries #132629 (comment)
• 2024-11-13 Error: Rollup of 5 pull requests #132997
• 2024-11-08 Error: Get rid of check_opaque_type_well_formed #132757 (comment)
• 2024-10-26 Error: Enable the llvm offload build configuration #131527 (comment)
• 2024-10-23 Error: Remove the Arc rt::init allocation for thread info #123550 (comment)

All on x86_64-mingw.

Can we maybe disable it on x86_64-mingw until it is fixed?

[jieyouxu]

I think the linker is crashing... Yeah let's disable it for x86-64-mingw.

EDIT: see #133481