Improve singleton!() macro #380
Merged
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
Right now, the `singleton!()` macro always creates a static named `VAR`.
This is annoying when digging through objdump output or digging into
memory with a debugger because it is hard to see what singleton you're
looking at when they are all called `<...>::{{closure}}::VAR`.
To make it a bit nicer, allow setting the name of the generated static
to some meaningful identifier which can then be cross-referenced in
debugger output, for example with
singleton!(FOO_BUFFER: [u8; 1024] = [0u8; 1024]);
There is no other side-effects to this change - the identifier is never
visible to other code because it is still contained in the closure of
the macro.
Rahix
force-pushed
the
improve-singleton-macro
branch
from
4de6d6f to
a02bce1
Compare
|
Can you add a CHANGELOG entry? |
thejpster
previously approved these changes
Jan 3, 2022
|
Added CHANGELOG entries. Should I send a second PR against the |
|
Thanks, this looks good to me too. Would you mind adding a comment inline to the macro definition briefly mentioning why it's a tuple with a bool instead of an Option, to help anyone coming along in the future with a great idea for an optimisation? I can take care of the backport once this is merged. I'll probably give it a little while though in case anything else comes along that can be included in a new 0.7 release since we only just pushed 0.7.4 a couple of days ago. |
As detailed in rust-embedded#364, niche optimization of the `Option` used in the `singleton!()` macro can lead to the initial value of the static to contain non-zero bits. This in turn leads to the whole static being moved from `.bss` to `.data` which means it eats up flash space for no reason. Especially if the singleton stores a particularly large type, this can be quite problematic. Prevent this by using an explicit boolean flag instead of the `Option` type. This is not quite as nice but at least there is no chance for the `singleton!()` to end up in `.data`...
Rahix
force-pushed
the
improve-singleton-macro
branch
from
ea2bd25 to
632af2e
Compare
|
@adamgreig: Good idea, done. |
|
Build succeeded: |
adamgreig
pushed a commit
that referenced
this pull request
May 15, 2022
380: Improve singleton!() macro r=adamgreig a=Rahix This PR addresses two shortcomings of the `cortex_m::singleton!()` macro, which I raised in #364. For review, I think it is best to look at the two commits implementing these changes individually. I think this changeset should also be backported to `0.7.x` where it applies cleanly and which is also the place where I tested it. The static is always initialized to a "zero" value with `Option::None` which means it should end up in `.bss`. However, if the enclosed type has a niche, `Option::None` can become a non-zero bitpattern which moves the whole singleton from `.bss` to `.data`. This is especially problematic when storing large buffers in the `singleton!()` as this starts eating lots of flash space unnecessarily. To prevent this, I switched to using an explicit boolean flag instead. This is not quite as nice but at least there is no chance for the `singleton!()` to end up in `.data`... For reference and as an example, the faulty behavior can be triggered with ```rust cortex_m::singleton!(: Option<u32> = None) ``` (the inner option has a non-zero niche which the outer option will occupy) Due to the static always being named `VAR` right now, all `singleton!()` instances end up having non-descriptive symbol names like `__cortex_m_rt_main::{{closure}}::VAR` which makes them hard to tell apart in a debugger or when looking at an objdump. I added the ability to set an explicit name which end up becoming part of the symbol name. This does not affect Rust code at all - the new name is not visible anywhere. It works like this: ```rust let value = singleton!(FOO_BUFFER: [u8; 1024] = [0u8; 1024]); ``` Of course the old syntax also still works and keeps the old behavior of calling the static `VAR`. Fixes #364. Co-authored-by: Rahix <[email protected]>
adamgreig
pushed a commit
that referenced
this pull request
May 15, 2022
380: Improve singleton!() macro r=adamgreig a=Rahix This PR addresses two shortcomings of the `cortex_m::singleton!()` macro, which I raised in #364. For review, I think it is best to look at the two commits implementing these changes individually. I think this changeset should also be backported to `0.7.x` where it applies cleanly and which is also the place where I tested it. The static is always initialized to a "zero" value with `Option::None` which means it should end up in `.bss`. However, if the enclosed type has a niche, `Option::None` can become a non-zero bitpattern which moves the whole singleton from `.bss` to `.data`. This is especially problematic when storing large buffers in the `singleton!()` as this starts eating lots of flash space unnecessarily. To prevent this, I switched to using an explicit boolean flag instead. This is not quite as nice but at least there is no chance for the `singleton!()` to end up in `.data`... For reference and as an example, the faulty behavior can be triggered with ```rust cortex_m::singleton!(: Option<u32> = None) ``` (the inner option has a non-zero niche which the outer option will occupy) Due to the static always being named `VAR` right now, all `singleton!()` instances end up having non-descriptive symbol names like `__cortex_m_rt_main::{{closure}}::VAR` which makes them hard to tell apart in a debugger or when looking at an objdump. I added the ability to set an explicit name which end up becoming part of the symbol name. This does not affect Rust code at all - the new name is not visible anywhere. It works like this: ```rust let value = singleton!(FOO_BUFFER: [u8; 1024] = [0u8; 1024]); ``` Of course the old syntax also still works and keeps the old behavior of calling the static `VAR`. Fixes #364. Co-authored-by: Rahix <[email protected]>
Merged
bors bot
added a commit
that referenced
this pull request
May 31, 2022
441: Prepare for v0.7.5 r=newAM a=adamgreig Currently with cortex-m 0.7.4 it's not possible for stable Rust users to enable the inline-asm feature, even though their compiler might support it, because of the `![cfg_attr(feature = "inline-asm", feature(asm))]` line. I propose a new 0.7.5 release that removes this line, which means users on stable Rust >=1.59 could enable the previously nightly-only feature to get stable inline asm. I wouldn't enable the feature by default, because that would be a significant MSRV bump, but at least this way more users could enable it before we release 0.8 with the revamped and inline-only asm. I've also backported the bugfix from #380 which I don't believe is a breaking change. I haven't had a chance to test this yet so it would be great if someone could try it out and just make sure the inline-asm feature does work before merging. Any thoughts on anything else worth backporting from 0.8? Co-authored-by: Adam Greig <[email protected]> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
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 PR addresses two shortcomings of the
cortex_m::singleton!()macro, which I raised in #364. For review, I think it is best to look at the two commits implementing these changes individually.I think this changeset should also be backported to
0.7.xwhere it applies cleanly and which is also the place where I tested it.1. Fix
singleton!()sometimes ending up in.dataThe static is always initialized to a "zero" value with
Option::Nonewhich means it should end up in.bss. However, if the enclosed type has a niche,Option::Nonecan become a non-zero bitpattern which moves the whole singleton from.bssto.data. This is especially problematic when storing large buffers in thesingleton!()as this starts eating lots of flash space unnecessarily.To prevent this, I switched to using an explicit boolean flag instead. This is not quite as nice but at least there is no chance for the
singleton!()to end up in.data...For reference and as an example, the faulty behavior can be triggered with
(the inner option has a non-zero niche which the outer option will occupy)
2. Allow naming the
staticDue to the static always being named
VARright now, allsingleton!()instances end up having non-descriptive symbol names like__cortex_m_rt_main::{{closure}}::VARwhich makes them hard to tell apart in a debugger or when looking at an objdump.I added the ability to set an explicit name which end up becoming part of the symbol name. This does not affect Rust code at all - the new name is not visible anywhere. It works like this:
Of course the old syntax also still works and keeps the old behavior of calling the static
VAR.Fixes #364.