Regression test for AVR `rjmp` offset #131755

jfrimmel · 2024-10-15T19:15:18Z

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

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)

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).

rustbot · 2024-10-15T19:15:26Z

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 · 2024-10-15T19:15:27Z

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

tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs

This fixes the [build error] caused by the `avr-gcc` (used as linker) not being available in the Rust CI. [build error]: rust-lang#131755 (comment)

tgross35 · 2024-10-15T23:01:47Z

r? compiler
cc @Patryk27

tests/run-make/avr-rjmp-offset/rmake.rs

This fixes the [build error] caused by the `avr-gcc` (used as linker) not being available in the Rust CI. [build error]: rust-lang#131755 (comment)

jfrimmel · 2024-10-16T08:59:52Z

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

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?
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 · 2024-10-16T10:19:48Z

What do you mean by suggested tool-flow?

jfrimmel · 2024-10-16T10:25:42Z

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 · 2024-10-16T10:59:35Z

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?

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.

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 · 2024-10-16T15:22:50Z

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.

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]: rust-lang#131755 (comment) [here]: rust-lang#131755 (comment)

jieyouxu

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

jfrimmel · 2024-10-16T18:38:32Z

Sorry for the noise, somehow plain lld works locally (with downloaded CI LLVM), but not in CI. I've tried to use the absolute path like the other tools (e.g. llvm_objdump()), but this didn't work as well. The newest code tries rust-lld, which also works locally, let's see...

jfrimmel · 2024-10-16T20:13:35Z

Ah, I see, other tests using "rust-lld" are using the following directive:

//@ needs-rust-lld

I can add this line, but this seems like there is no rust-lld available in CI, hence this test (and others) will never be executed. Am I missing something?

I didn't expect the linking to be the hard part... 😒

jieyouxu · 2024-10-17T04:14:00Z

I can add this line, but this seems like there is no rust-lld available in CI, hence this test (and others) will never be executed. Am I missing something?

I believe rust-lld is an alias to lld, but it should be available under some CI configurations. I'm asking T-infra about which CI runners have lld available.

EDIT: @jfrimmel I believe there are some CI jobs that have rust-lld, because e.g.

rust/tests/run-make/rust-lld-custom-target/rmake.rs

Line 8 in dd51276

//@ needs-rust-lld

several tests use that directive, and previously we did a survey on tests that never get exercised in (full) CI, and these //@ needs-rust-lld were at least exercised in 1 CI job.

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]: rust-lang#131755 (comment) [here]: rust-lang#131755 (comment)

jfrimmel · 2024-10-17T07:04:08Z

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 · 2024-10-17T10:18:06Z

@bors r+ rollup=iffy (linker dependent)

bors · 2024-10-17T10:18:08Z

📌 Commit a35ed2f has been approved by jieyouxu

It is now in the queue for this repository.

jieyouxu · 2024-10-17T17:59:24Z

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

jieyouxu · 2024-10-18T06:08:09Z

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 · 2024-10-18T06:08:12Z

📌 Commit a35ed2f has been approved by jieyouxu

It is now in the queue for this repository.

Rollup of 9 pull requests Successful merges: - rust-lang#130136 (Partially stabilize const_pin) - rust-lang#131755 (Regression test for AVR `rjmp` offset) - rust-lang#131774 (Add getentropy for RTEMS) - rust-lang#131802 (Dont ICE when computing coverage of synthetic async closure body) - rust-lang#131809 (Fix predicate signatures in retain_mut docs) - rust-lang#131858 (Remove outdated documentation for `repeat_n`) - rust-lang#131866 (Avoid use imports in `thread_local_inner!`) - rust-lang#131874 (Default to the medium code model on OpenHarmony LoongArch target) - rust-lang#131877 (checktools.sh: add link to issue for more context about disabled Miri tests) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-10-18T13:41:43Z

⌛ Testing commit a35ed2f with merge 1350eea...

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

jieyouxu · 2024-10-18T13:44:27Z

@bors r- (bors???)

jfrimmel added 3 commits October 15, 2024 12:22

Add check-annotations ensuring correct label

652ba66

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).

Allow the compiler to select any register

db6c736

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024

rustbot added the O-AVR Target: AVR processors (ATtiny, ATmega, etc.) label Oct 15, 2024

jfrimmel commented Oct 15, 2024

View reviewed changes

tests/run-make/avr-rjmp-offset/avr-rjmp-offsets.rs Outdated Show resolved Hide resolved

jfrimmel force-pushed the avr-rjmp-offset-regression-test branch from 0426aaa to 070e838 Compare October 15, 2024 19:58

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

jfrimmel added a commit to jfrimmel/rust that referenced this pull request Oct 15, 2024

Don't link the artifact to not depend on avr-gcc

a71fe55

This fixes the [build error] caused by the `avr-gcc` (used as linker) not being available in the Rust CI. [build error]: rust-lang#131755 (comment)

This comment has been minimized.

Sign in to view

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 15, 2024

rustbot assigned nnethercote Oct 15, 2024

jieyouxu reviewed Oct 16, 2024

View reviewed changes

tests/run-make/avr-rjmp-offset/rmake.rs Show resolved Hide resolved

jfrimmel added a commit to jfrimmel/rust that referenced this pull request Oct 16, 2024

Don't link the artifact to not depend on avr-gcc

5beb86c

This fixes the [build error] caused by the `avr-gcc` (used as linker) not being available in the Rust CI. [build error]: rust-lang#131755 (comment)

jfrimmel force-pushed the avr-rjmp-offset-regression-test branch from a71fe55 to 5beb86c Compare October 16, 2024 07:45

This comment has been minimized.

Sign in to view

jfrimmel force-pushed the avr-rjmp-offset-regression-test branch from 5beb86c to 4920c6a Compare October 16, 2024 15:51

jieyouxu approved these changes Oct 16, 2024

View reviewed changes

jieyouxu self-assigned this Oct 16, 2024

jfrimmel force-pushed the avr-rjmp-offset-regression-test branch from 8f5555f to 2e87739 Compare October 16, 2024 18:30

This comment has been minimized.

Sign in to view

jfrimmel mentioned this pull request Oct 16, 2024

[DO NOT MERGE] CI test #131803

Closed

jfrimmel force-pushed the avr-rjmp-offset-regression-test branch from 2e87739 to a35ed2f Compare October 17, 2024 07:00

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 17, 2024

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 17, 2024

jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2024

jieyouxu mentioned this pull request Oct 18, 2024

Rollup of 9 pull requests #131887

Merged

bors merged commit ebff167 into rust-lang:master Oct 18, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Oct 18, 2024

jfrimmel deleted the avr-rjmp-offset-regression-test branch October 18, 2024 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression test for AVR `rjmp` offset #131755

Regression test for AVR `rjmp` offset #131755

jfrimmel commented Oct 15, 2024 •

edited

Loading

rustbot commented Oct 15, 2024

rustbot commented Oct 15, 2024

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

tgross35 commented Oct 15, 2024

This comment has been minimized.

jfrimmel commented Oct 16, 2024 •

edited

Loading

jieyouxu commented Oct 16, 2024 •

edited

Loading

jfrimmel commented Oct 16, 2024

jieyouxu commented Oct 16, 2024 •

edited

Loading

jfrimmel commented Oct 16, 2024

jieyouxu left a comment

jfrimmel commented Oct 16, 2024

This comment has been minimized.

jfrimmel commented Oct 16, 2024 •

edited

Loading

jieyouxu commented Oct 17, 2024 •

edited

Loading

jfrimmel commented Oct 17, 2024

jieyouxu commented Oct 17, 2024

bors commented Oct 17, 2024

jieyouxu commented Oct 17, 2024 •

edited

Loading

jieyouxu commented Oct 18, 2024

bors commented Oct 18, 2024

bors commented Oct 18, 2024

jieyouxu commented Oct 18, 2024

Regression test for AVR rjmp offset #131755

Regression test for AVR rjmp offset #131755

Conversation

jfrimmel commented Oct 15, 2024 • edited Loading

rustbot commented Oct 15, 2024

rustbot commented Oct 15, 2024

This comment has been minimized.

This comment was marked as resolved.

This comment has been minimized.

tgross35 commented Oct 15, 2024

This comment has been minimized.

jfrimmel commented Oct 16, 2024 • edited Loading

jieyouxu commented Oct 16, 2024 • edited Loading

jfrimmel commented Oct 16, 2024

jieyouxu commented Oct 16, 2024 • edited Loading

Footnotes

jfrimmel commented Oct 16, 2024

jieyouxu left a comment

Choose a reason for hiding this comment

jfrimmel commented Oct 16, 2024

This comment has been minimized.

jfrimmel commented Oct 16, 2024 • edited Loading

jieyouxu commented Oct 17, 2024 • edited Loading

jfrimmel commented Oct 17, 2024

jieyouxu commented Oct 17, 2024

bors commented Oct 17, 2024

jieyouxu commented Oct 17, 2024 • edited Loading

jieyouxu commented Oct 18, 2024

bors commented Oct 18, 2024

bors commented Oct 18, 2024

jieyouxu commented Oct 18, 2024

Regression test for AVR `rjmp` offset #131755

Regression test for AVR `rjmp` offset #131755

jfrimmel commented Oct 15, 2024 •

edited

Loading

jfrimmel commented Oct 16, 2024 •

edited

Loading

jieyouxu commented Oct 16, 2024 •

edited

Loading

jieyouxu commented Oct 16, 2024 •

edited

Loading

jfrimmel commented Oct 16, 2024 •

edited

Loading

jieyouxu commented Oct 17, 2024 •

edited

Loading

jieyouxu commented Oct 17, 2024 •

edited

Loading