Fix link failure on AVR (incompatible ISA error)

[LuigiPiucco]

Fixes #137739. A reproducer of the issue is present there. I believe the root cause was introducing the avr-none target (which has no CPU by default) while also trying to get the ISA revision from the target spec. This commit uses the target-cpu option instead, which is already required to be present for the target.

r? compiler
cc @Patryk27

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

[Patryk27]

Ah, that's a nice one - wonder why my earlier tests worked then 🤔 Anyway, good to have it fixed.

[workingjubilee]

r? @workingjubilee

[LuigiPiucco]

wonder why my earlier tests worked then

Did you happen to test with atmega328p? That one linked properly, I guess it's still a default somewhere. Or maybe you built something that didn't require linking with avr-gcc/avr-ld? In any case, your work is already really good, the change toward avr-none was a nice improvement over having to write JSON specs for everything.

[workingjubilee]

@bors r+ rollup

[bors]

📌 Commit 4c1f51b has been approved by workingjubilee

It is now in the queue for this repository.

[LuigiPiucco]

Hum, I proposed this fix, but now I wonder if this approach would be better. It ensures the generic CPU default is replaced everywhere, instead of doing it just for the AVR note. Doing it like that would also help with something separate I have in mind, an implementation of which I will propose later. In avr-device, it would help a lot if we had a target_cpu cfg, so that ISA and peripheral definitions would always be in sync, like in avr-gcc. So if this alternative approach would be accepted, I'd prefer to replace this PR with that, such that a later PR can implement the cfg. Would that be ok? @workingjubilee

[workingjubilee]

target_cpu cfg

that would be a new Rust feature which has not been discussed before, I cannot sign off on that on my own, even experimentally.

[workingjubilee]

@LuigiPiucco There are a lot of nuances to why we have some target-cpu values set, that are very much not generic at all, but rather surprisingly often important for the soundness of all programs, and setting a different one makes all programs that exercise Rust's primitive types unsound, including the ones that target that CPU. Evaluating that change that affects all targets would require a much longer review cycle because I would have to work through all the implications in the backend. If you opened the PR now, I might be able to have it reviewed on March 3rd, but also depending on some things I haven't checked yet, it might be reviewed sometime in April.

And the other possible work would likely require discussion of the language-level implications, such as e.g. we might now need to start recognizing every target_cpu we accepted on our own, for every architecture, not just AVR, and as such could not simply support whatever the backend supports, but would instead have to identify these ourselves.

I also expect we would have requests, immediately, to approach it in a "tiered" fashion where future versions outmode, even though that's not right for every architecture, and so on. I don't think that's the right approach, but we'd need a response for why. That all requires a lot of discussion as to how do we respond to this feature request and how do we respond to related feature requests before even a provisional implementation should land.

So... I would not belay this change that fixes the target on something much more complicated.

[workingjubilee]

Well, I suppose it's not complicated in implementation, arguably even simpler, just... complicated in implications.

My apologies, I know this all might seem surprising and annoying, but, well, you see, 32-bit x86 exists, you know? So we all have to live with the consequences of its... very... organic development.

[LuigiPiucco]

So, to make it clear, the alternative approach would also increase review time, or would that only be the case if we're talking about the target_cpu cfg feature? If it's the former, I can propose it in another thread, as well. My intent was not to try and batch unrelated changes here, sorry if it sounded like that.

My reasoning for the alternative implementation was that, instead of using the target option just for the note, correcting the target spec's target.options.cpu with the value of the -C target_cpu=* option when present would make more sense; In the branch I linked, that's done for all architectures, but it could be exclusive to AVR to minimize the effect, and later discussed whether it should be extended to other cases.

target_cpu cfg is a separate matter entirely, I assume I'd need to go through an RFC for that, since it's a new feature, right? I commented on it because the alternative fix makes it easier to reason about, but the goal was not getting approval on that part right now, it was just added benefit to correcting the target spec.

[workingjubilee]

So, to make it clear, the alternative approach would also increase review time,

Yes.

I don't think it's particularly distinguishable from this changeset if it's just for AVR, and even if it is, I'd like to have a deeper discussion of, and time for, examining the reasoning and impacts beyond "it fixes this bug".

[LuigiPiucco]

I see, no worries. I'll open an issue later, and keep this PR the same.