Assert that mutexes and prints are not use in interrupt or critical context #4389
Conversation
|
This requires #4388 to pass testing. |
|
Are the changes to the RTX header files going to conflict with #4294 ? |
|
@theotherjimmy I'll hold off a bit on merging this to prevent conflicts with #4294. Worst case, if I need to merge this I'll rebase #4294 myself. |
There was a problem hiding this comment.
if we have are_interrupts_enabled, should this new function be is_isr_active or is_in_isr ? The former sounds better.
Regarding changes to RTOS, we are changing RTX internals that works the way that they return rather than assert. What disadvantages were for having asserts in our Mutex class, and keep internals as they are? Whoever touches directly RTX, should know what they are doing, probably same could be said for cmsis os layer.
|
retest uvisor |
|
RTX2 is doing the checks (if isr) by itself eg https://github.com/ARMmbed/mbed-os/blob/feature_cmsis5/rtos/rtx5/TARGET_CORTEX_M/rtx_mutex.c#L457 |
I forgot that they added events so users should be find out what is with the system. Based on this, should assert be only in C++, and keep RTX as it is (my above comment) ? |
|
I think there's no need to modify RTX5. |
|
I'm not sure to understand the purpose of this PR. The functions Adding assertion at this point do break the API. Not running in ISR it not a precondition for calling these function (even if it will fail), with this change, it become one. Users referring to CMSIS RTOS documentation might fall into that trap where a valid code become invalid because of that change. For low level |
|
Hi @pan-, the goal here is to trap on errors immediately if the code is wrong. Using a mutex in an interrupt handler indicates that there is an error in the program. In mbed-os 2 calling _read and _write in an interrupt handler resulted in undefined behavior. If you have any good use cases in mind where it is valid to use a mutex in an interrupt handler let me know, as that means this PR would be a problematic breaking change to the API. |
|
Looks like this needs a conflict resolved and to only be enabled when in debug build. This change should not modify behavior of default or release builds. |
c1728p9
force-pushed
the
assert_mutex_not_in_isr
branch
from
20e20f4 to
af7f3c7
Compare
|
Rebased on top of RTX5, changed Only top 9 commits are relevent. The rest are are for RTX5. This PR needs to be rebased on top of the main line RTX5 changes before running CI. |
Add the function core_util_in_isr() so code can determine if it is running in the context of an ISR.
Trigger an assert if a file is read from or written to from an interrupt handler or critical section. This includes using printf from an interrupt handler or critical section. This makes failures due to use in incorrect context deterministic and easier to locate. This feature is enabled by defining MBED_TRAP_ERRORS_ENABLED to 1 or by using the debug profile.
Wrap the file mbed_rtos_storage.h in extern "C". This allows the functions inside rtx_lib.h to have correct definitions when included in a C++ file. This is required for the RTX5 error trapping.
If MBED_TRAP_ERRORS_ENABLED is defined to 1 then trap on RTX errors. This includes using mutexes in ISR context.
Only allow error to be called once. This prevents a loop where error() calls exit() which in turn triggers another call to exit().
Add the attribute flash to enable priority inheritance and robust mode. The robust flag allows mutexes held by terminated threads to be properly released.
Prevent osTheadTerminate from being called on an already terminated thread. Also make sure the thread termination process is properly synchronized.
Add a .mbedignore to the storage_abstraction test since this is deprecated.
Define MBED_TRAP_ERRORS_ENABLED to 1 for the debug profile so errors are obvious when building as debug.
c1728p9
force-pushed
the
assert_mutex_not_in_isr
branch
from
af7f3c7 to
737c5a9
Compare
|
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
Add a function to determine if you are in ISR context, add asserts for context to mutex and printf usage and increase error buffer size so long asserts are printed correctly.