8000 cpu/arm7: Fix undefined behavior based on invalid assembly by Teufelchen1 · Pull Request #19619 · RIOT-OS/RIOT · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cpu/arm7: Fix undefined behavior based on invalid assembly #19619

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

8000
Jump to bottom
Merged
merged 1 commit into from
May 20, 2023
Merged

cpu/arm7: Fix undefined behavior based on invalid assembly #19619

merged 1 commit into from
May 20, 2023

Conversation

Teufelchen1
Copy link
Contributor

Contribution description

Hellooo! 🦤

In this PR a single NOP is added after an ldm in ARM7 common code in the scheduling management.
The change is necessary because this particular ldm affects certain banked registers, including R14 / lr. The next instruction then immediately accesses lr. This is invalid and the exact effect depends on the underlying hardware implementation. An intermediate NOP ensures correct behaviour in such cases.

The ARM docs can be found here, in section A4.1.21 LDM (2). It states:

In ARM architecture versions earlier than ARMv6, this form of LDM must not be
followed by an instruction that accesses banked registers. A following NOP is a good
way to ensure this.

Testing procedure

Without this change, on the Game Boy Advance, RIOT behaves irregularly. With the NOP, it works fine!
If possible, this change should be tested on other ARM7 hardware as well but I do not have access to any.

Special thanks @pyropeter for spotting the extremely tiny note on this issue within the ARM docs. You saved me hours of tears! 👾

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels May 19, 2023
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 20, 2023
@riot-ci
Copy link
riot-ci commented May 20, 2023

Murdock results

✔️ PASSED

583a1da cpu/arm7: Fix undefined behavior based on invalid assembly

Success Failures Total Runtime
6946 0 6946 11m:02s

Artifacts

@aabadie
Copy link
Contributor
aabadie commented May 20, 2023

bors merge

@bors
Copy link
Contributor
bors bot commented May 20, 2023

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.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 06e27d5 into RIOT-OS:master May 20, 2023
@Teufelchen1 Teufelchen1 mentioned this pull request Jun 5, 2023
Closed
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
Release 2023.07
Development

Successfully merging this pull request may close these issues.
5 participants
@Teufelchen1 @riot-ci @aabadie @maribu @benpicco
0