Repository navigation
feat(libc): implement std::_Atomic_assert - #7851
hypermoist wants to merge 6 commits into
Conversation
Write the console's message, the expression followed by " in function" and the function name when one is given, to stderr and abort. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
fogydes
left a comment
There was a problem hiding this comment.
Reviewed the diff:
- Replaces the legacy placeholder stub in
core/libs/prx/libc/Export.cppwith a clean Dinkumware implementation inStdioExtras.cpp, writingexpressionand optionalfunctionto stderr before callingstd::abort(). - Removes the non-compliant legacy comment from
Export.cpp. GuestAtomicAssert.cppusespipe()/fork()/waitpid()to verify child process abort behavior and output formatting, and is correctly gated underif(UNIX)inlibc/CMakeLists.txtso Windows builds are unaffected.- Zero comments added in code; Conventional Commits title; all checklist items verified.
Ready to merge.
AI-assisted: yes
|
Windows run of 727c28a merged onto
If Windows coverage is wanted, a variant could spawn itself with AI-assisted: yes (Claude Code). The results above were run on this machine. Edit: corrected the base commit from 585075c to 3611cda, the |
|
@Athen2045 thanks for the Windows run and the negative control. Good to have confirmation that the message line matches on Windows, with the |
|
Hi @hypermoist, a merge-order note for #9583 ( AI-assisted: yes |
|
@DavidSamir that works for me. Neither PR depends on the other, and whichever merges second keeps |
…sert Resolved the conflict in core/libs/prx/libc/Export.cpp by dropping the stub, as this branch implements it in StdioExtras.cpp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I checked this against the SDK 7.00
AI-assisted: yes |
MarcZX10
left a comment
There was a problem hiding this comment.
I’m updating my earlier approval after checking the SDK 7.00 behavior report in the disassembly comparison against the current stream wrappers.
[P2] Write the diagnostic through the guest stderr stream. The implementation calls host std::fputs(..., stderr), which bypasses the guest _Stderr FileStream. The reported retail routine calls the module’s fputs using _Stderr, so guest freopen/setvbuf state is honored. In this tree __stderrp_nid_postfix points at _Stderr_nid_postfix, and fputs_nid_postfix routes through the guest FileStream; please use that guest stream path (including its status/locking behavior) before aborting. Add a regression that redirects guest stderr and verifies the assertion text reaches the redirected stream; the existing fork test only redirects the host descriptor and therefore cannot detect this mismatch.
Please also resolve the null expression behavior against the same SDK reference: this PR substitutes an empty string, whereas the reported console routine passes the pointer to fputs. Don’t add a made-up success or message for that input; document and test the supported behavior consistent with the project’s conventions and the observed ABI.
Route diagnostic output through the guest _Stderr stream rather than host stderr, honoring redirection and buffering. Pass expression directly to fputs so null input faults/throws instead of substituting an empty string, document the technical debt, and test guest stderr redirection and null input behavior. Assisted-by: Antigravity:gemini-3.8-flash
…sert-work # Conflicts: # core/libs/prx/libc/CMakeLists.txt # docs/dev/debt/libc.md
|
@MarcZX10 @verusferro thanks for the reviews. Both requests are addressed in 5ff0e82:
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The new head resolves the two earlier findings: the diagnostic uses guest _Stderr, redirected guest stderr is tested, and the null-expression path is no longer replaced with an empty string.
What I ran
Exact head: 5ff0e821f6fea62e78559d73572821bd7e2e844f. Current upstream base: ba5b5330b37724bca4b99f30faab504653635a04. The current-main merge preview completed cleanly (tree 92810a64d035efd27bb35de0da1cedfc204175e3). Environment: Linux x86-64, GCC 12.2.0.
g++ -std=c++20 -I/tmp/AnyPS5-mention-head.ciGWED -Icore/libs -fsyntax-only /tmp/AnyPS5-mention-head.ciGWED/StdioExtras.cpp
g++ -std=c++20 -I/tmp/AnyPS5-mention-head.ciGWED -Icore/libs -fsyntax-only /tmp/AnyPS5-mention-head.ciGWED/GuestAtomicAssert.cpp
git diff --check fe2b52c7e431969782351d7d53fe0f7f9e0f344b..refs/review/mention-7851 -- core/libs/prx/libc/src/StdioExtras.cpp core/libs/tests/GuestAtomicAssert.cpp docs/dev/debt/libc.mdResults
| Check | Result |
|---|---|
GCC syntax-only: StdioExtras.cpp, GuestAtomicAssert.cpp |
Passed locally |
git diff --check |
Passed locally (exit 0) |
| Current-main merge preview | Clean; tree SHA above |
guest_atomic_assert |
Author reports passing on this exact head in the follow-up |
| Hosted checks | Eligibility, conventions, labels, report, and relinker jobs passed; Linux and Windows product jobs were skipped |
| Local CTest | Not run: the existing build-review configuration is relinker-only and has no guest_atomic_assert_tests target |
Raw local check output
StdioExtras.cpp: PASS
GuestAtomicAssert.cpp: PASS
7851 diff-check exit=0
92810a64d035efd27bb35de0da1cedfc204175e3
ninja: error: unknown target 'guest_atomic_assert_tests'
No tests were found!!!
Negative control
| Assertion | Code mutation | Result |
|---|---|---|
| Not run | No mutation-based negative control was run on this head | Not exercised |
Findings
Blocking
None found in the reviewed changes.
Non-blocking
The full local target and product build were not available in this relinker-only build configuration. The author reports the focused test on the exact head; hosted product build jobs were skipped.
Sources
- Guest
_Stderrimplementation at this head - Redirection and null-expression tests at this head
- Recorded behavior limits at this head
Not verified
I did not run the focused test or a full product build locally, and did not verify console behavior beyond the linked SDK 7.00 evidence recorded by the PR.
Correction — follow-up link fixedI mistyped the issue-comment ID in my review; the author's exact-head test report is here. The review decision and other reported results are unchanged. |
|
Static review plus an independent Linux run (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja, Release, What I checked
Focused test run
The CMake entry is scoped |
What
Implements
std::_Atomic_assert(const char* expression, const char* function)(_ZSt14_Atomic_assertPKcS0_,u2tMGOLaqnE), which was aNotImplementedstub inExport.cpp. It now sits next to_Assertinsrc/StdioExtras.cppand follows the same pattern: write the message to stderr, then abort.The message format comes from the retail
libc.prx(J03848509), which writesexpression, then" in function "andfunctionwhenfunctionis non-null, then"\n". The console brackets this withsceKernelDebugRaiseExceptionOnReleaseMode(0xa0020007, 0)andsceKernelDebugRaiseException(0xa0020007, 0)followed byud2. libc can't reach libkernel, so this usesstd::abort()like_Assert. A nullexpressionprints nothing for it instead of faulting.Tested
Linux (CachyOS, GCC Release). The new
guest_atomic_asserttest forks, calls the export with and without a function name, and checks the exact stderr text and SIGABRT. All 474guest_*tests pass. The NID drops off the libc todo list intools/progress.py.tools/check_conventions.pypasses.tools/pr_overlap.py --branchreports no line overlap. The only shared hunk is the libc test CMake, which #6044 moves along with every other test.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code