Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

IllegalInstruction exception when using embassy in release mode #3

Closed
TheButlah opened this issue Oct 4, 2022 · 6 comments
Closed

Comments

@TheButlah
Copy link

TheButlah commented Oct 4, 2022

I opened an issue in embassy for this but I believe it actually belongs here.
Here is a minimally reproducible example.

Now, my understanding of all of this is very limited so take what I say with a grain of salt, but it appears as though either the atomics are not actually properly emulated, or that there is some sort of low level safety issue happening.

I have been able to confirm that if I don't use embassy and just try using a combination of core::sync::atomic and critical_section::Mutex and do stuff, I wasn't able to reproduce the issue. So using embassy appears to be the best way to reproduce the bug.

The bug only occurs when compiling for the -imac instruction set, and only when embassy is built with opt-level = 1 or higher. This code works as expected in opt-level = 0.

In case its helpful, here is some additional context from the esp-rs matrix server, where @Dirbaio did some digging into alternative workarounds that might make the trap handler obsolete. Please note that regardless, there is still a bug in the atomic trap handler

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 5, 2022

The issue is caused by multiple problems - also in esp-hal!

However I found the problem here:

pub pc: usize, // pc, x0 is useless

The slot for X0 is used for the PC in the trap-frame but that breaks emulation of instruction that use x0 as an nice way to safe a register when dealing with zero (e.g. amoswap.w.aqrl a0,zero,(a0))

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 5, 2022

The issue for the problem in esp-hal is here: esp-rs/esp-hal#206
I have a fix ready but need #4 merged and released before

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 5, 2022

@TheButlah The two mentioned PRs should (together) fix the issue. You can try it locally by patching the dependencies. Your example worked for me this way but would be great if you could give it a try

@TheButlah
Copy link
Author

Will try soon! Y'all are awesome, thank you so much for putting up with my barrage of matrix messages ❤

@TheButlah
Copy link
Author

TheButlah commented Oct 6, 2022

Btw I saw somewhere that this would cause problems with esp-wifi, is that true? I was going to try out esp-wifi next

@TheButlah
Copy link
Author

I have tested by patching esp32c3-hal to the latest commit, and that pulls in the new version of the trap handler. I can confirm that it fixes the problem. Thanks!

Repository owner moved this from Todo to Done in esp-rs Oct 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

2 participants