-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve compatibility with ARM ACLE #2649
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
Improve compatibility with ARM ACLE #2649
Conversation
The sync.h header includes some definitions of some ARM ACLE intrinsics that can collide in breaking ways. This change improves compatibility in several ways: * If <arm_acle.h> exists, use that. This catches cases where builtins are not properly caught by __has_builtin(). * Fix function signatures for __dmb(), __dsb(), and __isb(), which should take a unsigned int as an argument according to the ARM ACLE. * Use __DMB(), __DSB(), and __ISB() throughout the code. This allows callsites to use the intrinsics without having to pass an argument, and matches well-established CMSIS patterns. * Provide a workaround for ARM host builds.
@@ -62,12 +62,19 @@ extern "C" { | |||
#endif | |||
#endif | |||
|
|||
#if (__ARM_ACLE >= 200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kilograham I imagine this will be true
for most compilers. Would you like an escape-hatch to make this branch testable?
Will this have any backwards-compatibility implications for already-written code that uses these functions in pico-sdk? |
Yes; anyone using the Anyone using the CMSIS-standard macro wrappers (e.g. |
yeah; we're not going to make a unilateral breaking change at this point, so keeping the __DMB() seems reasonable, but maybe guarding the ACLE stuff with an enabling define (note that we already guard some other stuff by inclusion of cmsis_core for example) "well established" ;-) -- you are the first to complain in 4 years. what is the particular use case out of curiosity |
^ not going to make a breaking change except in a major release |
I think I found more surgical fixes, going to close this PR. |
Split into these two PRs since they're technically not tied together:
The fact that the current signatures of __dsb(), __dmb(), and __isb() don't match the builtins isn't a real problem today, so punting on that for now since it's (hopefully) the only breaking API part of this change. |
The sync.h header includes some definitions of some ARM ACLE intrinsics that can collide in breaking ways. This change improves compatibility in several ways: