Skip to content

cortex-m-rt: Remove LR push, to ensure the stack is 8-byte aligned. #467

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Feb 13, 2023

This was causing incorrect execution of code optimized with the assumption the stack is 8-byte aligned.

Alternate version of #463

  • Remove instead of fix the sentinel/fake frame.
  • Remove code initializing LR, since it's now clobbered by the bl main anyway.
  • Remove the .cfi directives, since Reset now has no correct CFI info. I think this is the "correct" thing to do here.
  • Initialize the frame pointer in R7 (suggestion from @jamesmunns)

@Dirbaio Dirbaio requested a review from a team as a code owner February 13, 2023 23:40
This was causing incorrect execution of code optimized with the assumption the stack is 8-byte aligned.
Copy link
Member

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, :shipit:

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On balance I want to go this route, instead of #463. Thanks for preparing all the PRs @Dirbaio, and thanks to everyone else who helped investigate, especially @Dirbaio and @jamesmunns, and @peter9477 for spotting the issue in the first place.

There's a lot of useful context, explanation, and research in the #463 thread, though, so future readers should check it out.

Apologies in advance to the probe-run team; I think in the end it's best to go directly to the solution we want to stick with, and hopefully it's not too bad to patch up probe-run to eliminate the warning it will start emitting.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 14, 2023

@bors bors bot merged commit bba5c33 into rust-embedded:master Feb 14, 2023
bors bot added a commit that referenced this pull request Feb 14, 2023
470: Prepare for cortex-m-rt v0.7.3 r=thalesfragoso a=adamgreig

This fixes the miscompilation in #467, so I'd like to release it as soon as possible.

Co-authored-by: Adam Greig <adam@adamgreig.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants