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

Remove final panics from stm32xx-sys on h7 and keep it that way #1711

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Apr 2, 2024

Clients generally assume that sys won't panic. Until recently there's been nothing to ensure that.

This commit rearranges sys a bit and disables its ability to generate panics at link time, using the userlib/no-panic feature. This works on our current pinned toolchain, but also on 1.79-ish current nightly, so it appears not to be super fragile.

drv/stm32xx-sys/src/main.rs Outdated Show resolved Hide resolved
@hawkw hawkw self-requested a review April 2, 2024 23:44
@cbiffle cbiffle force-pushed the cbiffle/no-panic-sys branch 4 times, most recently from afbbf4c to 0dd39d8 Compare April 3, 2024 23:45
@cbiffle cbiffle marked this pull request as ready for review April 3, 2024 23:45
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This is great! I had a couple small suggestions, but no blockers. Let me know what you think?

@@ -37,6 +37,7 @@ g070 = ["family-stm32g0", "stm32g0/stm32g070", "drv-stm32xx-sys-api/g070", "drv-
g0b1 = ["family-stm32g0", "stm32g0/stm32g0b1", "drv-stm32xx-sys-api/g0b1", "drv-stm32xx-gpio-common/model-stm32g0b1"]

no-ipc-counters = ["idol/no-counters"]
no-panic = ["userlib/no-panic"]
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for making this a task feature, rather than just always enabling it? It seems a bit awkward to have to enable no-panic in the app.tomls, when the sys task will never panic, and this only controls the elimination of the panicking code. Since the task is built independently of other tasks, we don't need to worry about e.g. feature unification enabling no-panic for everyone, so I'd kind of prefer for the app.toml author to not have to think about this at all.

I'm open to being convinced otherwise about this, though, if you think there's a real use-case for not enabling no-panic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The sys task may never panic, but the compiler is only known to reliably remove all of the panics on some of our ARMv7-M builds. I haven't tested it at all on v6-M. So for now, it's conditional.

Definitely interested in forcing it on later once we've convinced ourselves it won't be annoying.

drv/stm32xx-sys/src/main.rs Outdated Show resolved Hide resolved
drv/stm32xx-sys/src/main.rs Show resolved Hide resolved
drv/stm32xx-sys/src/main.rs Outdated Show resolved Hide resolved
At least in some configurations.
@cbiffle cbiffle merged commit 858c297 into master Apr 4, 2024
103 checks passed
@cbiffle cbiffle deleted the cbiffle/no-panic-sys branch April 4, 2024 19:36
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.

None yet

2 participants