cortex-m
cortex-m copied to clipboard
rust-embedded
Incorrect implementation of `cortex_m::peripheral::scb::VectActive::from()` in stable release 0.7.7
I discovered this bug accidentally while working with SCB and exceptions. cortex_m::peripheral::scb::VectActive::from() function returns incorrect value, which is 16 higher than the expected value. (in case when VectActive is VectActive::Interrupt.
I know this was already fixed in #332, but it was really long time ago (2 years ago) and this change is not present in the current released version uploaded on crates.io.
https://github.com/rust-embedded/cortex-m/blob/bb4a78208323260a161e68b2498438867f971bc5/src/peripheral/scb.rs#L303C15-L303C15
Since the fix is already implemented in the master branch, I am curious about the timeline for the next release on crates.io. Alternatively, is it feasible to release a patch for version 0.7.7 specifically to address this bug?
It looks like 0.7.7 does the right thing?
https://github.com/rust-embedded/cortex-m/blob/v0.7.7/src/peripheral/scb.rs#L192
We can look by example, 303 line is https://github.com/rust-embedded/cortex-m/blob/bb4a78208323260a161e68b2498438867f971bc5/src/peripheral/scb.rs#L303 So, it returns Interrupt with value irqn, while irqn is greater or equal to 16.
In contrast, we have another function SCB::vect_active() which does the same (converting number to VectActive) and therefore should have the same implementation, but in fact it differs: https://github.com/rust-embedded/cortex-m/blob/bb4a78208323260a161e68b2498438867f971bc5/src/peripheral/scb.rs#L192
It was fixed in this commit: https://github.com/rust-embedded/cortex-m/pull/373/files#diff-0b00857297ead7a504fbfc9ffcaf8f62aad5996d563bc0b1ff5d5084e0769df8R303
together with replacing u8 with u16
Ugh, good spot.
Why do we have the same code pasted twice? fn vect_active() can just call .into(), surely?
I guess so, but the only difference is that we'll need .unwrap() the Option, and it's better to keep guard if irqn >= 16 just in case
Thanks for reporting this! I think we should backport the - 16 fix to 0.7.x and release 0.7.8 with it, but keeping u8 for now as it would be a breaking change otherwise. Would you like to open a PR (against the v0.7.x branch)? If there's a nice way to combine the vect_active implementation too then go for it, but i agree it's nice to avoid the unwrap and keep the guard.
@adamgreig, Sure, I can do it.
But how can we avoid using unwrap when we must cover all possible values in match? I guess it's ok to believe the register value we got from reading current vector register and assume correct arm target selected by user. Panic on incorrect vector value would be a better approach than undefined behavior we can get in the current implementation of vect_active function (for example if we got secure fault on target other than armv8, we got irqn - 16 = 7 - 16 => overflow in release build). Not totally sure such case is possible but seems like a good approach for safety to panic on unknown value.