Revert change of __time_critical to __no_inline #3169

Merged

kilograham merged 5 commits from no-inline-time-critical into develop

2026-09-04 12:06:02 -05:00

kilograham commented

2026-09-03 12:54:27 -05:00

(Migrated from github.com)

This seems (@will-v-pi) to have been an inadvertent change as part of 2.3.0

Whilst you could argue that time_critical should not be inlined, making it no-inline is a change in behavior, and causes a compiler warning/error if the user had "inline"d it before. Worse, if the warning is ignored, then the a link error happens with both the inline and __noinline (the function is called, but never created)

I think reverting the behavior makes most sense (we have __no_inline_not_in_flash_func vs __not_in_flash_func for a reason after all).... TLDR if you apply inline to one of the placement functions, it gets inlined not placed.

This seems (@will-v-pi) to have been an inadvertent change as part of 2.3.0 Whilst you could argue that time_critical should not be inlined, making it no-inline is a change in behavior, and causes a compiler warning/error if the user had "inline"d it before. Worse, if the warning is ignored, then the a link error happens with both the inline and `__noinline` (the function is called, but never created) I think reverting the behavior makes most sense (we have `__no_inline_not_in_flash_func` vs `__not_in_flash_func` for a reason after all).... TLDR if you apply `inline` to one of the placement functions, it gets inlined not placed.

peterharperuk (Migrated from github.com) reviewed

2026-09-03 12:54:27 -05:00

will-v-pi commented

2026-09-03 12:59:07 -05:00

(Migrated from github.com)

This change was intentional, to ensure time critical code is placed in the correct location (eg xip_sram), and not inlined into another location.

I would be against reverting it unless there are bugs, in which case if it is reverted the tests may need changing to add no_inline to their time critical code.

This change was intentional, to ensure time critical code is placed in the correct location (eg xip_sram), and not inlined into another location. I would be against reverting it unless there are bugs, in which case if it is reverted the tests may need changing to add no_inline to their time critical code.

peterharperuk commented

2026-09-03 13:40:38 -05:00

(Migrated from github.com)

I think I agree, __time_critical_func requires a physical function call so marking a function inline and __time_critical_func is a bug

kilograham commented

2026-09-03 17:21:31 -05:00

(Migrated from github.com)

per my comment, changing the behavior of __time_critical_func is not something we should do, as it affects pre-existing code and code generation (it potentially adds an actual function call between two possible preivously mutually inlined __time_critical funcs where there was none before)... we can argue the merits of whether the above is a wise scenario, but people do and have done it.

arguably defining inline __time_critical_func is pretty meaningless, however we cant prevent that without breaking the other (hence this issue)

The best compromise i can think of is to add __no_inline_time_critical_func() which i have done... @will-v-pi can you please use this func in places you depend on that behavior and push to this commit

per my comment, changing the behavior of `__time_critical_func` is not something we should do, as it affects pre-existing code and code generation (it potentially adds an actual function call between two possible preivously mutually inlined `__time_critical` funcs where there was none before)... we can argue the merits of whether the above is a wise scenario, but people do and have done it. arguably defining `inline __time_critical_func` is pretty meaningless, however we cant prevent that without breaking the other (hence this issue) The best compromise i can think of is to add `__no_inline_time_critical_func()` which i have done... @will-v-pi can you please use this func in places you depend on that behavior and push to this commit

will-v-pi commented

2026-09-04 05:36:46 -05:00

(Migrated from github.com)

I think we should also add a macro like __sdk_time_critical_func and replaces all uses inside the SDK with that, then add a PICO_TIME_CRITICAL_SDK_NO_INLINE or similar to toggle that between being defined as __time_critical_func and __no_inline_time_critical_func, so that the user can still force internal SDK time critical functions to be placed in the correct location?

e.g. something like this 0852f12853

I think we should also add a macro like `__sdk_time_critical_func` and replaces all uses inside the SDK with that, then add a `PICO_TIME_CRITICAL_SDK_NO_INLINE` or similar to toggle that between being defined as `__time_critical_func` and `__no_inline_time_critical_func`, so that the user can still force internal SDK time critical functions to be placed in the correct location? e.g. something like this 0852f128537e42750b8e6bda162d2e9278f10cf0

will-v-pi (Migrated from github.com) approved these changes

2026-09-04 10:33:00 -05:00