[AVR] Replace broken 'avr-unknown-unknown' target with 'avr-unknown-gnu-atmega328' target #74941
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @oli-obk (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
dylanmckay
force-pushed
the
replace-broken-avr-unknown-unknown-target
branch
from
ed8e34f
to
2ec4b70
Compare
|
cc @rust-lang/compiler this PR replaces a Tier 3 target that apparently never worked anyway with a controller specific tier 3 target that mostly serves as a reference target for others to copy and reuse |
dylanmckay
force-pushed
the
replace-broken-avr-unknown-unknown-target
branch
from
4749465
to
60bd312
Compare
|
☔ The latest upstream changes (presumably #75070) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sounds good to me, I say land it. |
|
@dylanmckay can you rebase and fix the conflicts? Then we can move to merging this |
LeSeulArtichaut
added
S-waiting-on-author
|
Ping from triage: @dylanmckay it looks like this PR can be merged when the conflicts are resolved. Could you rebase your branch? Thanks! |
…nu-atmega328' target The `avr-unknown-unknown` target has never worked correctly, always trying to invoke the host linker and failing. It aimed to be a mirror of AVR-GCC's default handling of the `avr-unknown-unknown' triple (assume bare minimum chip features, silently skip linking runtime libraries, etc). This behaviour is broken-by-default as it will cause a miscompiled executable when flashed. This patch improves the AVR builtin target specifications to instead expose only a 'avr-unknown-gnu-atmega328' target. This target system is `gnu`, as it uses the AVR-GCC frontend along with avr-binutils. The target triple ABI is 'atmega328'. In the future, it should be possible to replace the dependency on AVR-GCC and binutils by using the in-progress AVR LLD and compiler-rt support. Perhaps at that point it would make sense to add an 'avr-unknown-unknown-atmega328' target as a better default when implemented. There is no current intention to add in-tree AVR target specifications for other AVR microcontrollers - this one can serve as a reference implementation for other devices via `rustc --print target-spec-json avr-unknown-gnu-atmega328p`. There should be no users of the existing 'avr-unknown-unknown' Rust target as a custom target specification JSON has always been recommended, and the avr-unknown-unknown target could never pass the linking step anyway.
In general, linking with libc is not required, only libgcc is needed. As suggested in the code review, a better option for libc support is by building it into rust-lang/libc directly. This also removes the '-Os' argument to the linker, which is a NOP.
…spec The 'freestanding' module was only ever used for AVR. It was an unnecessary layer of abstraction. This commit merges the 'freestanding_base' module into 'avr_gnu_base'.
…own' to 'avr-unknown-gnu-atmega328'
dylanmckay
force-pushed
the
replace-broken-avr-unknown-unknown-target
branch
from
60bd312
to
a0905ce
Compare
…in platform-support.md
|
The branch has now been rebased. |
|
@bors r+ |
|
📌 Commit c9ead8c has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit c9ead8c with merge ca9f78dd98835a40aede3b7593cabf425c5bee21... |
|
💔 Test failed - checks-actions |
bors
removed
the
S-waiting-on-bors
bors
added
the
S-waiting-on-review
|
Strange, seemingly unrelated "command not found" error from bors https://github.com/rust-lang-ci/rust/runs/1032158434#step:23:22604 ( |
|
It's spurious failure. |
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
dylanmckay
added a commit
to avr-rust/book.avr-rust.com
that referenced
this pull request
Sep 3, 2020
This was added to Rust in rust-lang/rust#74941
dylanmckay
added a commit
to avr-rust/book.avr-rust.com
that referenced
this pull request
Sep 3, 2020
This was added to Rust in rust-lang/rust#74941
|
Since |
It does not - here it is only meant to indicate that this triple specificaly targets the GNU toolchain/linker/libraries. |
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Oct 4, 2024
…rochenkov Fix `target_env` in `avr-unknown-gnu-atmega328` The target name itself contains GNU, we should probably reflect that as `target_env = "gnu"` as well? Or from my reading of rust-lang#74941 (comment), perhaps not, but then that should probably be documented somewhere? There's no listed target maintainer, but the target was introduced in rust-lang#74941, so I'll ping the author of that: `@dylanmckay` Relatedly, I wonder _why_ the recommendation is to [create separate target triples for each AVR](https://github.com/Rahix/avr-hal/tree/main/avr-specs), when `-Ctarget-cpu=...` would suffice, perhaps you could also elaborate on that? Was it just because `-Ctarget-cpu=...` didn't exist back then? If so, now that it does, should we now change the target back to e.g. `avr-unknown-none-gnu`, and require the user to set `-Ctarget-cpu=...` instead?
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 5, 2024
Rollup merge of rust-lang#131171 - madsmtm:target-info-avr-env, r=petrochenkov Fix `target_env` in `avr-unknown-gnu-atmega328` The target name itself contains GNU, we should probably reflect that as `target_env = "gnu"` as well? Or from my reading of rust-lang#74941 (comment), perhaps not, but then that should probably be documented somewhere? There's no listed target maintainer, but the target was introduced in rust-lang#74941, so I'll ping the author of that: `@dylanmckay` Relatedly, I wonder _why_ the recommendation is to [create separate target triples for each AVR](https://github.com/Rahix/avr-hal/tree/main/avr-specs), when `-Ctarget-cpu=...` would suffice, perhaps you could also elaborate on that? Was it just because `-Ctarget-cpu=...` didn't exist back then? If so, now that it does, should we now change the target back to e.g. `avr-unknown-none-gnu`, and require the user to set `-Ctarget-cpu=...` instead?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
avr-unknown-unknowntarget has never worked correctly, always trying to invokethe host linker and failing. It aimed to be a mirror of AVR-GCC's
default handling of the `avr-unknown-unknown' triple (assume bare
minimum chip features, silently skip linking runtime libraries, etc).
This behaviour is broken-by-default as it will cause a miscompiled executable
when flashed.
This patch improves the AVR builtin target specifications to instead
expose only a 'avr-unknown-gnu-atmega328' target. This target system is
gnu, as it uses the AVR-GCC frontend along with avr-binutils. Thetarget triple ABI is 'atmega328'.
In the future, it should be possible to replace the dependency on
AVR-GCC and binutils by using the in-progress AVR LLD and compiler-rt support.
Perhaps at that point it would make sense to add an
'avr-unknown-unknown-atmega328' target as a better default when
implemented.
There is no current intention to add in-tree AVR target specifications for other
AVR microcontrollers - this one can serve as a reference implementation
for other devices via
rustc --print target-spec-json avr-unknown-gnu-atmega328p.There should be no users of the existing 'avr-unknown-unknown' Rust
target as a custom target specification JSON has always been
recommended, and the avr-unknown-unknown target could never pass the
linking step anyway.