Fix unset e_flags in ELF files generated for AVR targets #106619
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon. Please see the contribution instructions for more information. |
rustbot
added
T-bootstrap
agausmann
changed the title
This comment has been minimized.
This comment has been minimized.
| @@ -172,6 +172,330 @@ pub(crate) fn create_object_file(sess: &Session) -> Option<write::Object<'static | |||
| let e_flags = elf::EF_RISCV_RVC | elf::EF_RISCV_FLOAT_ABI_DOUBLE; | |||
| e_flags | |||
| } | |||
| Architecture::Avr => { | |||
| // Adapted from llvm-project/llvm/lib/target/AVR/AVRDevices.td | |||
| match sess.target.options.cpu.as_ref() { | |||
There was a problem hiding this comment.
It looks like we only support atmega328 at the moment. For AVR we pass a hard-coded linker flag (-mmcu=atmega328 for avr-unknown-gnu-atmega328) containing which AVR variant is used to the linker. I can imagine that it going out of sync with -Ctarget-cpu is a really bad idea. Instead it did probably need a separate target spec for each cpu. Especially if you can't mix a standard library compiled for one variant with one compiled for another variant. Maybe this EF_AVR_MACH_* constant could be specified to the target spec instead of using a giant match? Or maybe not? By the way, should we forbid -Ctarget-cpu for AVR?
There was a problem hiding this comment.
It looks like we only support atmega328 at the moment
Instead it did probably need a separate target spec for each cpu
Only atmega328 is in mainline rustc, but the Rust-AVR community supports many more using custom target specs.
For AVR we pass a hard-coded linker flag
And we add an -mmcu=* linker flag in our custom target specs too.
Maybe this
EF_AVR_MACH_*constant could be specified to the target spec instead of using a giant match?
Both AVR-GCC and LLVM accept the CPU name as a single flag (e.g. -mmcu) and use that to look up values like EF_AVR_MACH_* internally (e.g. with the AVRDevices.td table that I mentioned in the comment).
That being said, being able to specify the ISA revision in the target spec is also a good idea. It would make maintenance in rustc a little easier, as rustc would only have to maintain the list of ISAs and not every CPU ever released.
(CC @Rahix - would it be acceptable to specify the ISA alongside the CPU in the target specs?)
should we forbid
-Ctarget-cpufor AVR?
I do not see a reason for doing that.
There was a problem hiding this comment.
I do not see a reason for doing that.
Does using say -Ctarget-cpu=avr1 for one crate and -Ctarget-cpu=avrtiny for another crate work? If not, they have to be treated as separate targets without an overlapping set of allowed values for -Ctarget-cpu. The way that rustc checks for compatibility is by checking that the target name is identical. It assumes that linking crates with different -Ctarget-cpu values works fine and then runs on any cpu implementing the union of whatever features these -Ctarget-cpu values imply.
There was a problem hiding this comment.
For AVR specifically, the main usecase I can think of is picking a common-denominator ISA for building a "portable" library. I don't know whether that's actually useful or not. Probably not, since we currently specify each AVR CPU as its own target anyway.
And sure, if you misuse that, you may end up with a broken executable that doesn't work on your platform. That's true for any target.
However, I don't think it would be productive to add more code to forbid it. It's not like it's a "footgun" that a developer would regularly encounter and accidentally set incorrectly without realizing.
There was a problem hiding this comment.
I was worried about say AVR2 cpu's not being able to run AVR1 code. That AVR1 cpu's can't run AVR2 code is obvious.
It's not like it's a "footgun" that a developer would regularly encounter and accidentally set incorrectly without realizing.
You might not realize that the standard library is compiled for an incompatible cpu even if you set -Ctarget-cpu for your own crates.
In any case I guess it is fine to leave it as is.
There was a problem hiding this comment.
(CC @Rahix - would it be acceptable to specify the ISA alongside the CPU in the target specs?)
I certainly wouldn't mind at all. However both gcc and llvm also seem to maintain the list of MCUs so I think it is just as reasonable to do the same in rustc.
Cargo.toml
Outdated
| @@ -115,5 +115,7 @@ rustc-std-workspace-core = { path = 'library/rustc-std-workspace-core' } | |||
| rustc-std-workspace-alloc = { path = 'library/rustc-std-workspace-alloc' } | |||
| rustc-std-workspace-std = { path = 'library/rustc-std-workspace-std' } | |||
|
|
|||
| object = { git = "https://github.com/agausmann/object.git", branch = "0.29.0-avr-flags" } | |||
There was a problem hiding this comment.
Seems your changes were released as object version 0.30.2.
There was a problem hiding this comment.
Yes, and rustc has been updated to object 0.30; rebasing this is on my to-do list 👍
agausmann
force-pushed
the
avr-object-file
branch
from
8e0130f to
b6885e9
Compare
Ultimately it's up to the reviewer, but personally I think it's a good idea to make this |
Yeah, that would make sense. |
|
@petrochenkov I considered |
|
Is that actually a problem? It has several external dependencies already. |
|
r? rust-lang/compiler |
Probably not, just could shift the dependency graph slightly. I figured dependency changes were worth pointing out. |
|
|
| "atmega3209" => elf::EF_AVR_ARCH_XMEGA3, | ||
| "atmega4808" => elf::EF_AVR_ARCH_XMEGA3, | ||
| "atmega4809" => elf::EF_AVR_ARCH_XMEGA3, | ||
| _ => 0, |
There was a problem hiding this comment.
Is this a reasonable fall-back value for an unknown CPU? That is, will the tooling continue working with the objects if this field stays zero?
If not, there are two alternatives to consider here – a compiler error or falling back to a common denominator that is (almost?) guaranteed to work whatever the CPU is specified.
There was a problem hiding this comment.
Is this a reasonable fall-back value for an unknown CPU?
I believe zero is the value used for "generic" or "unspecified" in this case. It's also the value that e_flags has on AVR without this patch.
For example, in the error message in #106576 (avr architecture of input file `/tmp/rustcmivxhx/symbols.o' is incompatible with avr:103 output) , the 103 is the expected e_flags value. If the value is zero, then the linker just prints avr, like at the beginning of the error message, implying that it's "generic" AVR.
Another example of the same error, but different values: avr:51 architecture of input file `main.o' is incompatible with avr output (source)
|
I think adding a |
nagisa
added
S-waiting-on-author
|
Oh, I see that has already been mentioned. |
|
☔ The latest upstream changes (presumably #110214) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@agausmann any updates? |
agausmann
force-pushed
the
avr-object-file
branch
from
b6885e9 to
a7158ec
Compare
|
Switching PR state to waiting on review. @agausmann you can also switch the review assignment with @rustbot ready |
rustbot
added
S-waiting-on-review
|
Ah ok, thanks for the help @apiraino |
|
I haven’t reviewed the list of MCU-specific options too carefully, but the rest seems good to me. @bors r+ |
bors
added
S-waiting-on-bors
Fix unset e_flags in ELF files generated for AVR targets Closes rust-lang#106576 ~~Sort-of blocked by gimli-rs/object#500~~ (merged) I'm not sure whether the list of AVR CPU names is okay here. Maybe it could be moved out-of-line to improve the readability of the function.
|
⌛ Testing commit a7158ec with merge 94a19fef75b3b032c6570ad529081106dbb18f07... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (af9df2f): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 660.129s -> 659.918s (-0.03%) |
Closes #106576
Sort-of blocked by gimli-rs/object#500(merged)I'm not sure whether the list of AVR CPU names is okay here. Maybe it could be moved out-of-line to improve the readability of the function.