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

Fix target_env in avr-unknown-gnu-atmega328 #131171

Merged
merged 1 commit into from
Oct 5, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Oct 2, 2024

The target name itself contains GNU, we should probably reflect that as target_env = "gnu" as well? Or from my reading of #74941 (comment), perhaps not, but then that should probably be documented somewhere?

There's no listed target maintainer, but the target was introduced in #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, 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?

The target name itself contains GNU, we should set that in the
environment as well.
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024

Relatedly, I wonder why the recommendation is to create separate target triples for each AVR, 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?

My understanding at present is that if these AVR targets contain specific ABI-affecting codegen options / target features differences, including -Ctarget-cpu=..., having different targets is less prone to accidentally mixing code compiled with different incompatible ABI from different codegen flags. Crates that gets compiled by distinct targets like with avr-atmega1280.json target spec will be declared incompatible by rustc against code that is from a different target spec like avr-atmega1280.json. See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Ctarget-feature.3D.2Bsoft-float.60.20considered.20harmful and related discussion.

@workingjubilee
Copy link
Member

I believe target_env = "gnu" is correct.

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

Relatedly, I wonder why the recommendation is to create separate target triples for each AVR, 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?

My understanding at present is that if these AVR targets contain specific ABI-affecting codegen options / target features differences, including -Ctarget-cpu=..., having different targets is less prone to accidentally mixing code compiled with different incompatible ABI from different codegen flags. Crates that gets compiled by distinct targets like with avr-atmega1280.json target spec will be declared incompatible by rustc against code that is from a different target spec like avr-atmega1280.json. See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60-Ctarget-feature.3D.2Bsoft-float.60.20considered.20harmful and related discussion.

Thanks for the clarification; IMO, that sounds like a bug in rustc or the linker, then, if we're allowing linking object files with different incompatible CPU types?

@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024

Thanks for the clarification; IMO, that sounds like a bug in rustc or the linker, then, if we're allowing linking object files with different incompatible CPU types?

This is a known general class of problems where we should indeed be tracking ABI-affecting target features / codegen options and detecting if we're trying to link (build? not sure exactly at what time the detection is going to happen) things together with incompatibile options. It needs further some design work and follow-up implementation, but a possible design was discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/stabilizing.20compiler.20flags.20for.20Rust-for-Linux.

(You can search the issue tracker for A-ABI flag and see how many of those we have)

@workingjubilee
Copy link
Member

cc @Rahix

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

I'll add that this is especially relevant now for cc, which would like to get some of its target information from rustc, and that's complicated in general by targets not living in tree.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 2, 2024

I'll add that this is especially relevant now for cc, which would like to get some of its target information from rustc, and that's complicated in general by targets not living in tree.

rustc supports arbitrary target specs, albeit not stable, so AFAIK you might have to do something like

&["--print=all-target-specs-json", "-Zunstable-options"],

Not sure exactly about your use case in cc specifically, probably worth opening a dedicated #t-compiler thread.

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

I'll add that this is especially relevant now for cc, which would like to get some of its target information from rustc, and that's complicated in general by targets not living in tree.

rustc supports arbitrary target specs, albeit not stable, so AFAIK you might have to do something like

&["--print=all-target-specs-json", "-Zunstable-options"],

Not sure exactly about your use case in cc specifically, probably worth opening a dedicated #t-compiler thread.

I meant, --print=all-target-specs-json does not (and cannot) include targets defined in external json files, such as is commonly done here, so supporting those targets in cc is harder.

@Rahix
Copy link

Rahix commented Oct 2, 2024

I think this is correct from my point of view. But just in case, cc @Patryk27 as he's way better versed in the details of the AVR target than me :)

Relatedly, I wonder why the recommendation is to create separate target triples for each AVR, 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?

So this is something that seems to have flown past me completely. I'll have to have a play with it to see if -Ctarget-cpu can fully replace our current custom targets. We're currently adding a few more things to the target spec but from a quick glance, the majority of this should be replaceable by compiler options.

@BoxyUwU
Copy link
Member

BoxyUwU commented Oct 2, 2024

r? compiler

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 2, 2024

I'll have to have a play with it to see if -Ctarget-cpu can fully replace our current custom targets. We're currently adding a few more things to the target spec but from a quick glance, the majority of this should be replaceable by compiler options.

Cool! I've opened Rahix/avr-hal#586 to track it in a more appropriate place than this PR, sorry for the indirection.

@Patryk27
Copy link
Contributor

Patryk27 commented Oct 3, 2024

I'm not sure on the original design, but I'd be up for using just -Ctarget-cpu - just note that different AVR targets need different pre-link-args as well 👀

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 3, 2024

I'm not sure on the original design, but I'd be up for using just -Ctarget-cpu - just note that different AVR targets need different pre-link-args as well 👀

I can't speak for T-compiler here, but I suspect that they'd be fine with a match target.cpu { "..." => "some linker flag" } in rustc_codegen_ssa (depending on what the differing linker flags are).

@workingjubilee
Copy link
Member

workingjubilee commented Oct 3, 2024

while also not capable of speaking with The Voice Of An Entire Team, if we had some folks sign up to be AVR maintainers officially, I think we'd be happy to be merge whatever small compiler changes are necessary. frankly, we'd finally have someone to stick with the caseload of AVR weirdness and making decisions on what is and is not a bug.

@Patryk27
Copy link
Contributor

Patryk27 commented Oct 3, 2024

Sure, I've been handling AVR bugs for quite a while now and I don't mind doing it officially 🙂

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 4, 2024

Sure, I've been handling AVR bugs for quite a while now and I don't mind doing it officially 🙂

Nice! Could you file a PR that adds src/doc/rustc/src/platform-support/avr-unknown-gnu-atmega328.md, following the template? (and also update the top-level table and set the correct TargetMetadata in the target file) (at least I think that's the process).

I can also do it myself, but I think you know a lot more about the target, so you're probably better suited for providing the information.

@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 4, 2024

📌 Commit 033fdda has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#130453 (Add x86_64-unknown-trusty as tier 3 target)
 - rust-lang#130518 (Stabilize the `map`/`value` methods on `ControlFlow`)
 - rust-lang#131116 (Increase Stack Size for AIX)
 - rust-lang#131171 (Fix `target_env` in `avr-unknown-gnu-atmega328`)
 - rust-lang#131174 (Fix `target_abi` in `sparc-unknown-none-elf`)
 - rust-lang#131177 ( Stabilize 5 `const_mut_refs`-dependent API)
 - rust-lang#131238 (Remove mw from triagebot.toml)
 - rust-lang#131240 (Fix typo in csky-unknown-linux-gnuabiv2.md)
 - rust-lang#131257 ([rustdoc] Fix list margins)
 - rust-lang#131264 (Fix some `pub(crate)` that were undetected bc of `#[instrument]`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ff57e0b into rust-lang:master Oct 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 5, 2024
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?
@madsmtm madsmtm deleted the target-info-avr-env branch October 5, 2024 02:03
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 13, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 13, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 13, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 14, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 19, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 20, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Patryk27 added a commit to Patryk27/rust that referenced this pull request Oct 20, 2024
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants