Skip to content
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.

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

Push and pop registers in system mode within IRQ handler. #199

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

Anders429
Copy link
Contributor

Fixes #198.

After comparing the IRQ handler, the only difference I could see was that registers were pushed and popped within System mode in 0.12, and they were not in 0.13. Apparently mgba (and no$gba, which I also tested against while debugging) works just fine either way, but running on real hardware does not work correctly.

I don't know the exact technical reason for why this way works and the other doesn't, but this at least gets us back to the same functionality that was present before #197.

By the way, for future reference, my process here was to add another dependency on the old bracer version (using bracer_1_2 = {package = "bracer", version = "=0.1.2"} in Cargo.toml, and manually injecting the yanked version into Cargo.lock), replacing the relevant IRQ code with what existed in version 0.12.0, and verifying that it worked on hardware (which it did). Then I just replaced each bracer_1_2 macro call with the bracer 0.3.1 equivalent until I found the change that no longer worked on hardware.

Copy link
Member

@Lokathor Lokathor left a comment

Choose a reason for hiding this comment

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

I have no explanation for why this is what fixes the actual hardware case, but i'm willing to marge it anyway.

@Lokathor Lokathor merged commit 5e5fd5c into rust-console:main Sep 16, 2024
1 check passed
@svkampen
Copy link

svkampen commented Sep 16, 2024

I'll have a go at an explanation!

In the non-working version, we have the following assembly:

 80001b4:       /-- beq 80001d8 <__runtime_irq_handler+0x4c>
 80001b8:       |   mrs r3, SPSR
 80001bc:       |   push        {r3, lr}
 80001c0:       |   msr CPSR_c, #223    @ 0xdf
 80001c4:       |   add lr, pc, #0
 80001c8:       |   bx  r12
 80001cc:       |   msr CPSR_c, #210    @ 0xd2
 80001d0:       |   pop {r3, lr}
 80001d4:       |   msr SPSR_fc, r3
 80001d8:       \-> bx  lr

Note that, specifically, lr is modified at 80001c4. It may seem like this is fine, we pushed it after all, but we pushed it before switching to system mode, and lr is a banked register (so the lr we have pushed is lr_irq, which is not the lr in system mode). Therefore, 80001c4 clobbers the system mode lr, which unsurprisingly breaks things.

Adding push {lr} and pop {lr} around the fake blx makes the old code work as well, but I suppose moving the push and pop of r3 into system mode is fine, as well.

Now, this doesn't explain why it does work in emulators. I have tried stepping through the mGBA debugger, but without a corresponding hardware debugger I obviously can't see where they diverge, and honestly given that this fixes the issue I'm personally not that invested in making mGBA reproduce the bug 😉

@Anders429
Copy link
Contributor Author

Thanks for the explanation! Makes me wonder if this is worth raising an issue on mGBA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting RUST_IRQ_HANDLER breaks interrupts on real hardware
3 participants