Fix thumbv4t-none-eabi frame pointer setting #99227
Conversation
rustbot
added
the
T-compiler
|
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Just chiming in to say that I also use thumbv4t-none-eabi and this seems like a good change to me. |
| main_needs_argc_argv: false, | ||
|
|
||
| // don't have atomic compare-and-swap | ||
| atomic_cas: false, | ||
| has_thumb_interworking: true, | ||
|
|
||
| ..super::thumb_base::opts() | ||
| ..Default::default() |
There was a problem hiding this comment.
I'm a little surprised from the PR description that this doesn't just manually set the frame pointer to "may omit" which thumb_base gets wrong for this specific target, but instead decides not to use thumb_base at all.
There was a problem hiding this comment.
Somehow that didn't even occur to me compared to "give me the real defaults".
There was a problem hiding this comment.
I think I'd prefer to see it still use thumb_base and override the changed field, rather than this approach. Unless there's a reason why this specific target is likely to diverge from thumb_base more often than not. I'm not super familiar with what we normally do for target specifications though, so others may disagree.
There was a problem hiding this comment.
I can set the field here, but either way I'd prefer to not use thumb_base. Then as the target maintainer i have to try and pay attention to changes to an additional thing. Also as a user a person would have to look in 3 places instead of just 2 to understand all the details.
There was a problem hiding this comment.
That's understandable, but at the same time, if there is a change to thumb_base that would be applicable here then it could easily be missed. Ultimately, this is a trade-off that's inherent to how we define these target specs, and I'm not sure it makes sense to be inconsistent about inheriting from base specs on the basis of the preference of the target maintainer, but I don't feel strongly about this. I'd like to see what others think.
|
@bors r+ |
bors
added
S-waiting-on-bors
Rollup of 8 pull requests Successful merges: - rust-lang#99227 (Fix thumbv4t-none-eabi frame pointer setting) - rust-lang#99518 (Let-else: break out scopes when a let-else pattern fails to match) - rust-lang#99671 (Suggest dereferencing index when trying to use a reference of usize as index) - rust-lang#99831 (Add Fuchsia platform support documentation) - rust-lang#99881 (fix ICE when computing codegen_fn_attrs on closure with non-fn parent) - rust-lang#99888 (Streamline lint checking) - rust-lang#99891 (Adjust an expr span to account for macros) - rust-lang#99904 (Cleanup html whitespace) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
As just one example, here's the previous output of a particular function where the only important line was and then this is what it looks like with the updated target profile in today's nightly: I'd call that excellent results |
The
thumb_baseprofile has changed since I last remember seeing it, and now it sets the frame pointer to "always keep", which is not desired for this target. Hooking a debugger to the running program is not really done, it's preferable to have the register available for actual program use, so the default "may omit" is now set.I thought that the target was already using "may omit" when I checked on it last month, because I forgot that the target was previously based on
thumb_baserather thanDefault::default(). I only noticed the issue just now when creating thearmv4t-none-eabitarget (#99226), though this PR is not in any way conditional on that one.