WIP: Improve compatibility with ARM ACLE #2649

2025-09-04T20:08:17-05:00

armandomontanez commented

2025-09-04 20:08:17 -05:00

(Migrated from github.com)

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.

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.

armandomontanez (Migrated from github.com) reviewed

2025-09-04 20:17:24 -05:00

src/rp2_common/hardware_sync/include/hardware/sync.h

					
				@ -62,12 +62,19 @@ extern "C" {

				#endif

				#endif

				#if (__ARM_ACLE >= 200)

@kilograham I imagine this will be true for most compilers. Would you like an escape-hatch to make this branch testable?

@kilograham I imagine this will be `true` for most compilers. Would you like an escape-hatch to make this branch testable?

lurch commented

2025-09-05 08:00:35 -05:00

(Migrated from github.com)

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.

Will this have any backwards-compatibility implications for already-written code that uses these functions in pico-sdk?

> * 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. Will this have any backwards-compatibility implications for already-written code that uses these functions in pico-sdk?

armandomontanez commented

2025-09-05 12:36:25 -05:00

(Migrated from github.com)

Yes; anyone using the __dsb(), __dmb(), and __isb() functions from the Pico SDK without passing an argument will see breakages due to the signature change. I'm not sure there's a satisfying workaround since the ARM ACLE spec explicitly states these builtins take an unsigned int, so any code that has arm_acle.h and hardware/sync.h may see conflicting definitions.

Anyone using the CMSIS-standard macro wrappers (e.g. __DMB()) will see no change.

Yes; anyone using the `__dsb()`, `__dmb()`, and `__isb()` functions from the Pico SDK without passing an argument will see breakages due to the signature change. I'm not sure there's a satisfying workaround since the ARM ACLE spec explicitly states these builtins take an `unsigned int`, so any code that has `arm_acle.h` and `hardware/sync.h` may see conflicting definitions. Anyone using the CMSIS-standard macro wrappers (e.g. `__DMB()`) will see no change.

kilograham commented

2025-09-08 13:07:47 -05:00

(Migrated from github.com)

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

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

kilograham commented

2025-09-08 13:08:13 -05:00

(Migrated from github.com)

^ not going to make a breaking change except in a major release

armandomontanez commented

2025-09-10 20:39:01 -05:00

(Migrated from github.com)

I think I found more surgical fixes, going to close this PR.

armandomontanez commented

2025-09-10 21:02:47 -05:00

(Migrated from github.com)

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.

Split into these two PRs since they're technically not tied together: * https://github.com/raspberrypi/pico-sdk/pull/2657 * https://github.com/raspberrypi/pico-sdk/pull/2658 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.