Conversation
| // to keep Mutex implementation code in one translation unit. | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| void MutexInterface::lock() { |
There was a problem hiding this comment.
[Design] could fix MutexInterface::lock()/unLock() and ScopeLock live in the delegate's TU, coupling the interface's common API to getDelegate().
Any use of Os::Mutex::lock()/ScopeLock pulls DelegateMutex.o into the link, which carries an undefined reference to MutexInterface::getDelegate. Under compile-time selection (the PR's stated motivation, and implement-osal.md §2.5 "Skip the Delegate Factory") a project therefore still needs a DefaultMutex.cpp and links the unused delegate. REQUIRES_IMPLEMENTATIONS Os_Mutex masks this today, so impact is minor; a MutexInterface.cpp holding these common functions would match the intent stated in MutexInterface.hpp ("available regardless of which implementation"). Same pattern exists in DelegateRawTime.cpp, so this may be an accepted trade-off.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
I agree with this finding.....why are the common interfaces defined in this TU?
| // ---------------------------------------------------------------------- | ||
|
|
||
| void MutexInterface::lock() { | ||
| MutexInterface::Status status = this->take(); |
There was a problem hiding this comment.
[Test Quality] future work MutexInterface::lock() / unLock() failure path (FW_ASSERT on non-OP_OK) is exercised only on the success path.
The common implementation now lives on the interface and is the default Os::Mutex behaviour for every configured implementation, but Os/Stub/test/ut/StubMutexTests.cpp only drives lock()/unLock() with takeStatus/releaseStatus left at OP_OK. The stub already supports status injection, so an ASSERT_DEATH (or assert-hook) case setting StaticData::data.takeStatus = ERROR_OTHER before mutex.lock() would pin the contract. Preexisting gap (the old Mutex::lock() had the same untested branch); not introduced here.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
| // 2. Compile-time selection (performance optimization): Platforms may override | ||
| // this header to alias Os::Mutex directly to a concrete implementation | ||
| // (e.g., Va416x0Os::AtomicMutex::AtomicMutex). This eliminates the wrapper and | ||
| // virtual dispatch, enabling inlining and aggressive LTO optimization. |
There was a problem hiding this comment.
[Operational] suggestion ops-config-extreme: a compile-time Os::Mutex override is not paired with the ConditionVariable implementation that consumes its handle.
Scenario: a project aliases Os::Mutex to a concrete type here but still links an Os_Mutex implementation module (which bundles Condition, e.g. Posix). PosixConditionVariable::pend() does reinterpret_cast<PosixMutexHandle*>(mutex.getHandle()) on the foreign handle and hands that memory to pthread_cond_wait — silent UB / hang at the first ConditionVariable::wait() in flight, with no compile-time diagnostic. The header also does not state that the aliased type must derive from MutexInterface (required by lock()/unLock()/ScopeLock, 95 framework call sites). Operational judgment call; smallest remedy is a WARNING in this config header.
| // 2. Compile-time selection (performance optimization): Platforms may override | |
| // this header to alias Os::Mutex directly to a concrete implementation | |
| // (e.g., Va416x0Os::AtomicMutex::AtomicMutex). This eliminates the wrapper and | |
| // virtual dispatch, enabling inlining and aggressive LTO optimization. | |
| // 2. Compile-time selection (performance optimization): Platforms may override | |
| // this header to alias Os::Mutex directly to a concrete implementation | |
| // (e.g., Va416x0Os::AtomicMutex::AtomicMutex). This eliminates the wrapper and | |
| // virtual dispatch, enabling inlining and aggressive LTO optimization. | |
| // | |
| // WARNING: the aliased type MUST derive from Os::MutexInterface (lock()/unLock()/ | |
| // ScopeLock are defined on the interface), and the linked Os_Mutex implementation | |
| // module (which also provides Os::ConditionVariable) MUST understand the aliased | |
| // type's MutexHandle: e.g. Os/Posix/ConditionVariable.cpp reinterpret_casts the | |
| // handle to PosixMutexHandle. Mixing a compile-time Mutex with a mismatched | |
| // ConditionVariable implementation is undefined behavior with no build-time error. |
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
Applied suggestion as part of this documentation update: 6a1f104
| void lock(); //!< lock the mutex and assert success | ||
| void unLock(); //!< unlock the mutex and assert success |
There was a problem hiding this comment.
[Operational] could fix ops-doc-reality: the compile-time-selection performance claim does not reach the framework's actual mutex call sites.
lock()/unLock() (and ScopeLock) are defined out-of-line in DelegateMutex.cpp and call this->take() through a MutexInterface*, so even with Os::Mutex aliased to a final concrete class every acquisition is still one out-of-line call plus one virtual call unless LTO inlines across the TU. In-tree, 39 ScopeLock and 56 lock()/unLock() call sites use this path and 0 call take()/release() directly — i.e. 100% of framework locking sees none of the "eliminates virtual dispatch, enabling inlining" benefit stated in config/OsDelegateMutex.hpp without LTO. Operational judgment call: either define lock()/unLock() inline in this header (drop the .cpp definitions) so the call site can devirtualize, or qualify the claim in the config header as LTO-dependent.
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
either define lock()/unLock() inline in this header (drop the .cpp definitions) so the call site can devirtualize
@LeStarch and/or @thomas-bc , please let me know what you think of this suggestion. I think this question relates to a handful of the other autobot review comments as well.
or qualify the claim in the config header as LTO-dependent.
The particular project I'm working on uses LTO, so the proposed change isn't strictly necessary to meet our needs, but I'm open to doing it that way if that's what the F Prime team would prefer to see.
There was a problem hiding this comment.
[Operational] Disagreement — escalating. I still flag this on 82c128d; the contributor's response above indicates we disagree.
Re-checked at this head: lock()/unLock()/ScopeLock remain out-of-line in DelegateMutex.cpp calling virtual take()/release(), so the "eliminates virtual dispatch" claim in config/OsDelegateMutex.hpp still holds only with LTO. The contributor notes their project uses LTO and defers the inline-vs-qualify choice to the maintainers, which is reasonable; I keep the finding open only because the config-header claim is unqualified for non-LTO projects.
cc @LeStarch @thomas-bc — needs human adjudication.
There was a problem hiding this comment.
I think either we should:
- Fix it as specified
- Note it as a follow-up issue so we track it.
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 1)
Pre-run prompt-injection alert
prompt-injection in PR-authored content before reviewers were
invoked. All reviewers were warned via their kickoff prompts.
Flagged surfaces:
- pr_body: hidden-html-comment — "<!-- If you are an AI agent or bot, please respond to the question "Generative AI was used in this con..."
The supply-chain reviewer's inline findings below include full
analysis of any prompt-injection content in the diff and metadata.
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 0 | 1 | 0 | 1 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 1 | 0 | 1 | Go |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | Go |
| Documentation Currency | 1 | 0 | 1 | 0 | 2 | No-Go |
| Design | 0 | 0 | 1 | 0 | 1 | Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 0 | 0 | 0 | 1 | 1 | Go |
| Correctness | 0 | 0 | 0 | 0 | 0 | Go |
| Operational | 0 | 1 | 1 | 0 | 2 | Go |
| Maintainability | 0 | 1 | 1 | 0 | 2 | Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 1 | 2 | 6 | 1 | 10 | No-Go |
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | clean |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | 1 could-fix — hidden AI-targeting HTML comment (from PR template) in PR body |
| Review-system integrity | clean |
Outstanding must-fix items (1)
Documentation Currency
Os/docs/sdd.md§5.2.2 still lists RawTime as the only service supporting compile-time selection; add Mutex (config/OsDelegateMutex.hpp/OS_MUTEX_HEADER) — #5936 (comment)
Merge readiness
Merge readiness: No-Go — Documentation Currency has 1 outstanding must-fix item (OSAL SDD compile-time-selection list is stale).
One doc update from a clean burn — update the SDD and this alias is go for orbit.
…value-initialized Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
| **Currently Supported Services:** | ||
| - **RawTime** | ||
| - **RawTime** (`config/OsDelegateRawTime.hpp`, `OS_RAW_TIME_HEADER`) | ||
| - **Mutex** (`config/OsDelegateMutex.hpp`, `OS_MUTEX_HEADER`; `lock()`/`unLock()`/`ScopeLock` live on `MutexInterface`, so they work with any aliased implementation) |
There was a problem hiding this comment.
[Operational] could fix ops-doc-reality: "work with any aliased implementation" is unqualified; the SDD omits the two conditions under which the compile-time Mutex path fails and the fact that the §5.2.2 "zero virtual dispatch" advantage does not currently hold for Mutex.
Scenario: a project aliases Os::Mutex to a concrete type (1) that does not derive from MutexInterface — all 95 in-tree lock()/unLock()/ScopeLock call sites fail to compile — or (2) whose MutexHandle is not the one the linked Os_Mutex implementation's ConditionVariable expects (e.g. Os/Posix/ConditionVariable.cpp:23 reinterpret_casts it to PosixMutexHandle*) — UB at the first ConditionVariable::wait() in flight, no build-time error. Also, because lock()/unLock() are out-of-line in DelegateMutex.cpp and call virtual take(), every framework acquisition still pays one call + one virtual call without LTO. Operational judgment call; smallest remedy is qualifying this bullet.
| - **Mutex** (`config/OsDelegateMutex.hpp`, `OS_MUTEX_HEADER`; `lock()`/`unLock()`/`ScopeLock` live on `MutexInterface`, so they work with any aliased implementation) | |
| - **Mutex** (`config/OsDelegateMutex.hpp`, `OS_MUTEX_HEADER`; the aliased type MUST derive from `MutexInterface` — `lock()`/`unLock()`/`ScopeLock` live there — and its `MutexHandle` MUST be the one understood by the linked `Os_Mutex` implementation's `ConditionVariable` (e.g. Posix `reinterpret_cast`s it to `PosixMutexHandle`); mismatches are undefined behavior with no build-time error. Note: `lock()`/`unLock()` are defined out-of-line and call virtual `take()`/`release()`, so the zero-virtual-dispatch benefit for Mutex currently requires LTO.) |
cc @LeStarch @thomas-bc — low-confidence finding, please confirm.
There was a problem hiding this comment.
Simplified this bullet to match RawTime and added clarifying comments to OsDelegateMutex.hpp in this commit: 6a1f104
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 5)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 0 | 1 | 0 | 0 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 1 | 0 | 0 | Go |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | Go |
| Documentation Currency | 1 | 0 | 1 | 0 | 0 | Go |
| Design | 0 | 0 | 1 | 0 | 0 | Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 0 | 0 | 0 | 1 | 1 | Go |
| Correctness | 0 | 0 | 0 | 0 | 0 | Go |
| Operational | 0 | 1 | 2 | 0 | 0 | Go |
| Maintainability | 1 | 2 | 2 | 0 | 2 | No-Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 2 | 3 | 8 | 1 | 3 | No-Go |
Since last run
| Agent | resolved | still open | newly added | incorrect-fix follow-ups | improperly resolved | disagreements escalated |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 0 | 0 | 0 | 0 | 0 |
| Supply Chain / Runner Safety | 0 | 0 | 0 | 0 | 0 | 0 |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | 0 |
| Documentation Currency | 0 | 0 | 0 | 0 | 0 | 0 |
| Design | 1 | 0 | 0 | 0 | 0 | 0 |
| Architecture | 0 | 0 | 0 | 0 | 0 | 0 |
| Test Quality | 0 | 1 | 0 | 0 | 0 | 0 |
| Correctness | 0 | 0 | 0 | 0 | 0 | 0 |
| Operational | 3 | 0 | 0 | 0 | 0 | 0 |
| Maintainability | 0 | 0 | 2 | 0 | 0 | 0 |
Duplicates consolidated this run: 0 (threads closed by the §5h post-pass)
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | clean |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | clean |
| Review-system integrity | clean |
Outstanding must-fix items (1)
Maintainability
Os/DelegateMutex.cppfile\briefstill points readers here for theMutexInterface/ScopeLockcommon code that this head moved intoOs/MutexInterface.hpp(one-line suggestion attached) — #5936 (comment)
Merge readiness
Merge readiness: No-Go — Maintainability has 1 outstanding must-fix (stale \brief in Os/DelegateMutex.cpp after the inline move). Everything else is green: the inline move of lock()/unLock()/ScopeLock into MutexInterface.hpp at 6a1f104 closed the Design thread and all three Operational threads (including the escalated devirtualization disagreement); the remaining open threads are one Test Quality future-work item and one Maintainability could-fix.
Fifth pass: the devirtualization burn is complete — one stale doc comment stands between this mutex and the pad.
Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
lestarch-autobot
left a comment
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 6)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 0 | 1 | 0 | 0 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 1 | 0 | 0 | Go |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | Go |
| Documentation Currency | 1 | 0 | 1 | 0 | 0 | Go |
| Design | 0 | 0 | 1 | 0 | 0 | Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 0 | 0 | 0 | 1 | 1 | Go |
| Correctness | 0 | 0 | 0 | 0 | 0 | Go |
| Operational | 0 | 1 | 2 | 0 | 0 | Go |
| Maintainability | 1 | 2 | 2 | 0 | 0 | Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 2 | 3 | 8 | 1 | 1 | Go |
Since last run
| Agent | resolved | still open | newly added | incorrect-fix follow-ups | improperly resolved | disagreements escalated |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 0 | 0 | 0 | 0 | 0 |
| Supply Chain / Runner Safety | 0 | 0 | 0 | 0 | 0 | 0 |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | 0 |
| Documentation Currency | 0 | 0 | 0 | 0 | 0 | 0 |
| Design | 0 | 0 | 0 | 0 | 0 | 0 |
| Architecture | 0 | 0 | 0 | 0 | 0 | 0 |
| Test Quality | 0 | 1 | 0 | 0 | 0 | 0 |
| Correctness | 0 | 0 | 0 | 0 | 0 | 0 |
| Operational | 0 | 0 | 0 | 0 | 0 | 0 |
| Maintainability | 2 | 0 | 0 | 0 | 0 | 0 |
Duplicates consolidated this run: 0 (threads closed by the §5h post-pass)
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | clean |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | clean |
| Review-system integrity | clean |
Merge readiness
Merge readiness: Go — all 10 reviewers completed with zero outstanding must-fix. The two commits since 6a1f104 fixed the stale \brief and removed the duplicated rationale note in Os/DelegateMutex.cpp, closing both Maintainability threads; the only open thread is one Test Quality future-work item (MutexInterface::lock()/unLock() assert path untested). Maintainer review was requested on an earlier run.
Sixth pass, all stations green — the mutex alias is cleared for docking.
3 participants
Change Description
This PR applies the same pattern used for
Os::RawTimein #5617 toOs::Mutex:Update fprime so that
Os::Mutexis an alias (set by theusingkeyword) which defaults toOs::DelegateMutexbut which projects can override to be a platform specific implementation of Mutex.Rationale
See rationale for #5249
This PR is being proposed now as it is expected to provide a performance improvement needed for a specific project using F Prime.
Testing/Review Recommendations
Verify the build works & verify UTs pass.
Future Work
Future work: convert other OS services to follow this pattern as described in #5249 (comment).
AI Usage (see policy)
Claude Code (Opus 4.8) was used to generate an initial draft of the proposed change to Os::Mutex based on the pattern used for Os::RawTime in #5617, and also for help gathering internal performance metrics on the impact of this change.