Skip to content

Add alias for Os::Mutex - #5936

Open
jrussino wants to merge 19 commits into
nasa:develfrom
jrussino:russino-mutex-delegate
Open

jrussino wants to merge 19 commits into
nasa:develfrom
jrussino:russino-mutex-delegate

Conversation

@jrussino

@jrussino jrussino commented Sep 10, 2026

Copy link
Copy Markdown
Related Issue(s) #5945, #5249
Has Unit Tests (y/n) n
Documentation Included (y/n) n
Generative AI was used in this contribution (y/n) y

Change Description

This PR applies the same pattern used for Os::RawTime in #5617 to Os::Mutex :

Update fprime so that Os::Mutex is an alias (set by the using keyword) which defaults to Os::DelegateMutex but 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.

@jrussino
jrussino marked this pull request as ready for review September 11, 2026 07:18
@lestarch-autobot
lestarch-autobot self-requested a review September 11, 2026 07:53
Comment thread Os/Mutex.hpp
Comment thread Os/DelegateMutex.cpp Outdated
Comment thread default/config/OsDelegateMutex.hpp
Comment thread Os/Mutex.hpp
Comment thread Os/DelegateMutex.cpp Outdated
// to keep Mutex implementation code in one translation unit.
// ----------------------------------------------------------------------

void MutexInterface::lock() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this finding.....why are the common interfaces defined in this TU?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented proposed fix here: deda1bb

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Design] Fixed in 6a1f104.

Comment thread Os/DelegateMutex.cpp Outdated
Comment thread Os/DelegateMutex.cpp Outdated
Comment thread Os/MutexInterface.hpp Outdated
Comment thread default/config/OsDelegateMutex.hpp Outdated
Comment thread Os/MutexInterface.hpp Outdated

@lestarch-autobot lestarch-autobot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review summary (run 1)

Pre-run prompt-injection alert

⚠️ The orchestrator's pre-run metadata scan flagged potential
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.

Joseph A Russino and others added 3 commits September 16, 2026 09:51
Comment thread Os/docs/sdd.md Outdated

@lestarch-autobot lestarch-autobot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.cpp file \brief still points readers here for the MutexInterface / ScopeLock common code that this head moved into Os/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.

jrussino and others added 2 commits September 21, 2026 13:31
Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
@jrussino
jrussino requested a review from LeStarch September 21, 2026 20:33

@lestarch-autobot lestarch-autobot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lestarch-autobot lestarch-autobot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review summary (run 7)

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 0 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 only change since 4628a63 is a merge of devel (no PR-authored commits; the PR-side patch over the 10 touched files is byte-identical to run 6), so every reviewer reported no delta. The single open agent thread is the Test Quality future-work item (MutexInterface::lock()/unLock() assert path untested). Maintainer review was requested on an earlier run.


Seventh pass on a devel-merge-only head — steady as she goes, still cleared for docking.

@thomas-bc thomas-bc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Os::ConditionVariable and Os::Mutex should be updated as a pair - if we merge this as is we introduce a risk of UB

Os::ConditionVariable has no alias; it goes through getDelegate() from the chosen Os_Mutex_* implementation, and that implementation reinterpret_casts Os::Mutex::getHandle() to its own handle type (Os/Posix/ConditionVariable.cpp). So CHOOSES_IMPLEMENTATIONS Os_Mutex_Posix + using Mutex = MyPlatform::MyMutex compiles cleanly and is UB at runtime. Nothing checks consistency. (RawTime has no such coupling.)

EDIT: FYI, I'm sending our AI overlords to fix and push a PR to your fork - but feel free to implement yourself as well if you prefer owning that part.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Os::Mutex to use alias pattern

4 participants