Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The current diff has no confirmed code defect, but the PR template is incomplete and the repository conventions check fails on required checklist fields.
What I ran
Reviewed head 76a68b0085539ae549cb6673d069b5c1ab85ddf9 against PR base 223b19ba0adb11b66893a57fa656ecf6bc4a45cf; I also previewed it against current main, 2b1ae87c69bff8ba1df7a708ed7d6b099d416a08, and it merges cleanly. Environment: Linux x86_64, Python 3.11.2, Git 2.39.5, GCC 12.2.0 available; no compiler or build was run.
git diff --check 223b19ba0adb11b66893a57fa656ecf6bc4a45cf...refs/review/13536
python3 tools/check_conventions.py --base 223b19ba0adb11b66893a57fa656ecf6bc4a45cf --head 76a68b0085539ae549cb6673d069b5c1ab85ddf9 --title 'fix(libc): export the rand seed state' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13536-76a68b008553/pr-body.md
python3 /tmp/anyps5-review-13536/core/libs/tests/test_public_headers.py --audit-only
python3 /marc/AnyPS5/tools/pr_test_quality.py --root /tmp/anyps5-review-13536 --json core/libs/tests/GuestMath.cpp
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base 223b19ba0adb11b66893a57fa656ecf6bc4a45cf --head 76a68b0085539ae549cb6673d069b5c1ab85ddf9 --json
python3 tools/pr_stack_check.py 13536 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-stack-13536-final
gh pr checks 13536 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Whitespace/diff check | Passed; exit 0, no output. |
| Conventions and PR template | Failed with three template errors: two unchecked required checklist items and a Depends on value that is not #N or none. |
| Public-header audit | Passed: 374 libraries and 23,703 exports passed unique-declaration and umbrella-coverage checks. This audit checks declarations; it does not validate the new data export in a guest import. |
| Test registration | Source inspection confirmed CMake registers GuestMath.cpp as guest_math. |
| Test-quality heuristic | Warned that GuestMath.cpp may be unregistered; this is a false positive because CMake builds the path using ${LIBS_INCLUDE_DIR}. |
| Export/ABI helper | Returned passed_with_warnings with three findings outside the changed files. It audits function declarations/definitions and does not verify this data symbol's guest export/import behavior. |
| Current-main merge preview | Passed; the exact head previews cleanly against 2b1ae87c69bff8ba1df7a708ed7d6b099d416a08. The branch is behind, but no rebase is requested solely for that because the preview is clean. |
| Hosted checks | Six policy/status checks were queued at the final refresh; none is a C++ runtime test. |
guest_math |
Not run locally; running it requires a build. |
Unedited relevant output
Conventions check:
(pull request): error: [pr-template] Fill in the pull request template: unchecked: Based on current `main`; no other open PR implements the same functions
(pull request): error: [pr-template] Fill in the pull request template: unchecked: Depends on: landing order with the shared-file PRs is not agreed
(pull request): error: [pr-template] Fill in the pull request template: fill Depends on with #N or none
3 findings, 3 errors; GitHub shows at most 10 error annotations, the job summary lists all
Public-header audit:
374 libraries, 23703 exports: unique declarations and umbrella coverage verified
Test-quality heuristic:
{
"findings": [
{
"path": "core/libs/tests/GuestMath.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}Merge preview report:
- PR #13536: clean against current base; head `76a68b0085539ae549cb6673d069b5c1ab85ddf9`; changed paths: none reported
No declared PR dependencies were found in the target PR chain.
Hosted checks:
conventions pending
eligibility pending
intake pending
labels pending
library-portability pending
report pending
Findings
Blocking
- The repository requires every PR-template line to be completed. The required “Based on current
main; no other open PR implements the same functions” item is unchecked, and the dependency item is unchecked with prose instead of the required#Nornonevalue. Please verify the current-main/overlap statement, then check it if accurate; list actual prerequisite PR numbers inDepends on, or writenoneif there are no dependencies, and check that item. The clean merge preview does not by itself establish a dependency or require a rebase.
Non-blocking
- The implementation and assertions consistently use one shared
_Randseedvariable, and the added test is registered. The standalone macOS fixture described in the PR does not exercise the patched guest-import path; that integration behavior remains unverified here, so the checks above do not establish that a guest import resolves the new data symbol.
Sources
- The
_Randseeddeclaration at the reviewed head - The shared state and
rand/srandimplementation at the reviewed head - The registered
guest_mathtarget at the reviewed head
Not verified
I did not build or run guest_math, independently reproduce the author-reported macOS fixture, verify the guest-import relocation at runtime, or test Windows, Linux runtime behavior, console behavior, or a game.
To get this to a good state
Complete the required checklist fields and rerun the conventions check. If the head changes, I will rerun these relevant checks against the new head.
|
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.
|
What
Expose
_Randseedas the state already used byrandandsrand. Previously those functions used a private variable, so an import of the guest data symbol could not observe or change the same state. The update formula, seed assignment and eight-byte initial value were checked in both version-labelled images; those labels do not authenticate firmware provenance.Tested
srandupdating the exported state,randupdating it, and a direct state write affecting the next result. This does not exercise the patched guest-import path or a full production build.rand/srand; future rebases may need reconciliation.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch).mdfiles or images added to the repository