sys/threads: assert on uninitialized mutex/cond handles

[xvuko]

This is first step in improving detection of uninitialized handles. Adding checks to non DEBUG builds needs to be considered, but requires more care before it can be safely adopted.

Motivation and Context

mutexLock is frequently used without checking return value. If mutex create is not invoked then all mutexLock invocations are silently ignored without any sign that locks are never actually held.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

• Already covered by automatic testing.
• New test added: (add PR link here).
• Tested by hand on: (list targets here).

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing linter checks and tests passed.
• My changes generate no new compilation warnings for any of the targets.

Special treatment

• This PR needs additional PRs to work:
  • sys: use phMutexLock and phCondWait when checking EINVAL phoenix-rtos-tests#496
• I will merge this PR by myself when appropriate.

[gemini-code-assist]

Code Review

This pull request introduces a static inline helper function handleIsValid to validate resource handles in sys/threads.c. It also adds assertions using this helper to verify the validity of handles in mutexLock, condWait, and mutexLock2. There are no review comments, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[github-actions]

Unit Test Results

12 336 tests   11 557 ✅  57m 46s ⏱️
   746 suites     777 💤
     1 files         2 ❌

For more details on these failures, see this check.

Results for commit 7fe9cc4.

♻️ This comment has been updated with latest results.

[julianuziemblo]

IIRC imxrt uses uninitialized mutex in phoenix-rtos/phoenix-rtos-devices/multi/imxrt-multi/gpio.c (which should be fixed)

[oI0ck]

Unsure about this one, maybe asserting things like these should be behind a different macro definition in addition to NDEBUG. I can imagine that this might break in unexpected places (which is a good thing in the end, since we shouldn't have uninitialized mutexes, and we often don't check the return value).

Making this opt-in for now would be good for preserving code containing improper uses, as long as it is not fixed. We can then roll this out to be enabled by default and opt-out.

[oI0ck]

Wouldn't a macro be more fitting here? Also, provide an alternative definition in #elsif

[xvuko]

#ifndef NDEBUG is needed here only to avoid unused function warnings. Note that at this point only llvm shows warning here as GCC disables unused check for all static inline functions regardless and not only those included from headers.

I prefer to avoid macros when possible. Here function provides better type safety without any drawbacks.

[julianuziemblo]

I'd still provide an alternate definition or state here that this has to be used only inside the assert macro

[xvuko]

Unsure about this one, maybe asserting things like these should be behind a different macro definition in addition to NDEBUG

NDEBUG is an existing mechanism of enabling optional runtime checks. I do not see need to create separate macro here. This is part of phoenix specific API, so it is used only in code we can fix. Note that POSIX locks are not affected as those have explicit handling of this case.

There are some further checks that would be nice to have ie. assert(err >= 0). Those could fail on some (in theory) valid use-cases so it would make sense to make them optional.

After this change is used for some time we could consider removing return value from default mutex API calls and raising trap from kernel. For now NDEBUG is a good way to ensure checks can be disabled on production, but are not missed in CI

[julianuziemblo]

this name could be somewhat misleading as we don't really check if it is valid for sure, only if it could be valid

[ziemleszcz]

Maybe reverse the logic (handleIsInvalid()) and add comment that currently false negative is possible.

[xvuko]

Changed to:

/* TODO: expose RESOURCE_ID_MIN from kernel headers */
#define HANDLE_MIN 1
...
assert(m >= HANDLE_MIN);

[ziemleszcz]

Maybe reverse the logic (handleIsInvalid()) and add comment that currently false negative is possible.