feat: configurable link section attribute for irqs #718
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
This change introduces a new config field that allows `svd2rust` to target which linker sections get assigned to the `__INTERRUPTS` static, with reasonable defaults. Previously on RISC-V, the choice was always left up to the compiler, and it seemed to always pick `.rodata`. Unfortunately, in my context, that meant placing the LUT in a memory range that had a lot of highly variable latency, which cost not just time but predictability in servicing interrupts. With this change in place, I'm able to target a particular section (e.g. `.data`, or `.trap.rodata`) for the placement of the static, which grants more granular control over the ultimate loaded memory address. For the full details about the problem, please see: esp-rs/esp-hal@e29f3d5
Replaces `get(0)` with `first()` as suggested by https://rust-lang.github.io/rust-clippy/master/index.html#get_first
sethp
added a commit
to rustbox/esp-pacs
that referenced
this pull request
May 19, 2023
This commit premptively adopts the change proposed to [svd2rust] to expose the link section as a config parameter. [svd2rust]: rust-embedded/svd2rust#718
sethp
added a commit
to rustbox/esp-pacs
that referenced
this pull request
May 19, 2023
This commit preemptively adopts the change proposed to [svd2rust] to expose the link section as a config parameter. [svd2rust]: rust-embedded/svd2rust#718
This was referenced May 19, 2023
burrbull
reviewed
May 19, 2023
|
I hope this is backward compatible? |
burrbull
previously approved these changes
May 19, 2023
|
Oh, great point: the code generator changes are in the sense that passing Using the new parameter may not be backwards compatible, depending on what link section is being set: as long as the new section gets output in some part of the resulting binary it sure could be, but it's somewhat easy to rely on a particular layout, either accidentally or on purpose. |
Thanks to @burrbull for the review!
burrbull
approved these changes
May 19, 2023
|
bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
jessebraham
pushed a commit
to esp-rs/esp-pacs
that referenced
this pull request
May 22, 2023
* feat: place __INTERRUPTS vector in `.trap.rodata` This commit preemptively adopts the change proposed to [svd2rust] to expose the link section as a config parameter. [svd2rust]: rust-embedded/svd2rust#718 * chore: update generated files via: ``` (cd xtask; cargo run -- --generate-only) ``` * fix: quiet rust-analyzer's noisy complaints Rust-analyzer's "all targets" default mode runs afoul of `#[no_std]` crates that don't have a test harness. This change tells cargo (and indirectly, rust-analyzer) that we have no expectation for that to work. The CI changes don't have any effect currently, since none of these crates have any `bin`s or `example`s, but it does check that the rust-analzyzer complaints won't crop back up in the future. * chore: point at post-merge-queue commit
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.
This change introduces a new config field that allows
svd2rustto target which linker sections get assigned to the__INTERRUPTSstatic, with reasonable defaults.Previously on RISC-V, the choice was always left up to the compiler, and it seemed to always pick
.rodata. Unfortunately, in my context, that meant placing the LUT in a memory range that had a lot of highly variable latency, which cost not just time but predictability in servicing interrupts.With this change in place, I'm able to target a particular section (e.g.
.data, or.trap.rodata) for the placement of the static, which grants more granular control over the ultimate loaded memory address.For the full details about the problem, please see: esp-rs/esp-hal@e29f3d5