Repository navigation
test(libkernel): cover sys and web timed join imports - #9582
UnhealthyKraken wants to merge 7 commits into
Conversation
|
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.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
This follow-up is scoped to removing the libkernel_sys throwing export and exercising the existing requested-variant-first fallback; it does not duplicate the canonical implementation in #8890. The Linux integration evidence attached to #8890 covers the canonical provider, and the Windows description reports same-binary baseline/candidate comparisons for sys and web plus the broader lifecycle suite. The dependency on #8890 is explicitly recorded and the prerequisite remains open, so the staged order is clear.
I checked the new Windows fixture code and found one coverage gap in the description: the invalid-deadline fixture sets an invalid thread value, but expects only the same EINVAL as the null-deadline and null-thread fixtures. This confirms invalid input does not crash and returns EINVAL, but does not distinguish invalid-deadline precedence from a thread-pointer validation path that also returns EINVAL. The canonical Linux test does check invalid deadline before null/self thread, but the PR claims that precedence for the module-scoped Windows imports too. Please add a case where the thread value would produce a different result if checked first (or phrase the Windows claim as a null/invalid argument check and rely on the canonical test for precedence).
Hosted eligibility, conventions, intake, labels and report checks pass; product Linux/Windows CI jobs are skipped. The Windows execution reports and Linux canonical results were provided by the PR authors; I did not run tests locally.
|
Independent Windows run of this PR, as data only. The machine is a Ryzen 9 5950X on Windows 11 (10.0.22621), built with WinLibs GCC 15.2.0 UCRT r7, Release,
Not done: the baseline arm, meaning your new fixtures against |
|
Thank you for the independent Windows run. My original baseline arm used the same generated fixture executables in both arms, changing only the requested variant DLL, with the canonical timed-join prerequisite held constant:
Those results were on the original recorded baseline (main 6ef5a1c plus the canonical prerequisite), not your newer d6732b5 base. Your 210/211 result and baseline-reproduced exception-test timeout remain separate attributed evidence; I have not counted the timeout as a passing test or run a title. |
There was a problem hiding this comment.
The follow-up is correctly separated from #8890: it removes the variant throwing stub and routes scoped imports through the already-declared canonical provider. The requested-module precedence remains intact. The Windows test script also passes py_compile, and I generated all eight sys/web fixture images locally; I did not execute those Windows fixtures. The earlier test-coverage point remains: invalid-deadline uses thread value 1 and expects EINVAL. That checks the current invalid pointer does not get dereferenced for this invalid deadline, but it cannot distinguish invalid-deadline precedence from another safe invalid-thread path that also returns EINVAL. Please either test the actual variant export with a valid self-thread handle and assert EINVAL rather than EDEADLK, or narrow the Windows claim to “invalid deadline with an invalid thread value does not crash”; the canonical test can remain the evidence for general precedence. Please also resolve current overlaps with their authors before merging: - #7149: it is still open and changes the native pthread join tail in Pthread/src/Thread.cpp, where #8890 moves the join implementation into the shared helper this PR uses. The description acknowledges this integration concern, but I do not see an agreed order with #7149's author or a dependency that records it. Agree the order and preserve both behaviors when rebasing. - #9713: this newer open PR edits the adjacent pthread_switch_* stubs in the same libkernel_sys/Export.Discovery.cpp block from which this PR removes pthread_timedjoin_np. The overlap report shows the shared hunk. Please coordinate an order/resolution with its author and document it. - #6044: its open test reorganization affects the shared test registrations and paths. Please coordinate how GuestTimedJoin.cpp and its CMake registrations will move if #6044 lands first. It is now 1,235. This branch is 107 commits behind current upstream/main. Because #8890 is still open and is the declared prerequisite, settle the overlap and prerequisite order first, then update to current main and rerun checks. Checks on exact head 6cf0937ccfd128f17612d348ba21a3d0113acf43: - git diff --check upstream/main...HEAD: passed. - python tools/check_conventions.py --base upstream/main --head HEAD: passed. - python3 -m py_compile core/libs/tests/test_windows_timed_join_imports.py: passed. - Generated the eight sys/web fixture images successfully; Windows execution was not attempted locally. - Hosted product build/test jobs are skipped. The detailed Windows runs in the PR and the independent Windows report are attributed reports, not local runs by me. I did not find a defect in the shared timed-join implementation during this review.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
|
Correction to my earlier review: I retract the concern about the invalid-deadline test being unable to distinguish precedence. I rechecked Exact-head checks I ran: I still request integration coordination and current-base refresh. Current |
UnhealthyKraken
changed the title
|
Refreshed onto main 329efe1 with history-preserving merges. #8890 is now 235ea5f; #9582 is da52d09. All 20 selected Windows suites pass, including actual sys/web imports, canonical normal/coarse timed join, lifecycle/cancellation, kernel-sys and patched-library loading. Conventions, registration (1176 executables, none unregistered) and whitespace pass. No Linux/macOS or console retest; the full repository suite was not run. The current conventions checker newly rejects the prerequisite alone while the duplicate sys discovery declaration exists. I therefore included that deletion in #8890; #9582 now retains the scoped-import regressions. The final combined tree is identical to the tested 4158a67 tree, and the regression-first baseline evidence remains in the PR description. No published history was rewritten. The invalid-deadline/valid-self precedence control is retained. Proposed order: #8890, then #9582. @Pouare514 (#7149), preserve your successful Windows join ReapRetiredWinStacks call in the shared success path, and the detach/exit retirement logic. @poonasor (#4563), the Reap-before-native-wait handshake similarly belongs in that shared path if your protocol lands, preserving AwaitReap and detach handling. @simonbronk (#9713), retain your five sys implementations and remove their discovery stubs together with the timed-join stub; this is an adjacent deletion conflict. @giannireale (#9770), your removal/reorganization of the adjacent registration block must retain the timed-join wrapper and correct registration. @TaymKal (#6044), if the test reorganization lands first, I will adapt the canonical and sys/web fixtures to its final paths/helpers/labels. Please confirm the intended landing order for these overlaps. These external branches were not combined/tested here, and agreement remains pending. The refreshed overlap scan reports #9713/#9770 textual conflicts and 22 base-conflicted shared-file warnings; their shared diffs were inspected, including the lifecycle interactions above. The generic intake hold was withdrawn, so I am not treating it as a blocker. The wider audit will pause after this review-follow-up pass. |
|
I’ve noticed that asking you to hold this PR was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. |
|
@MarcZX10 Acknowledged; the submission hold is withdrawn. The corrected precedence evidence and the refreshed #8890/#9582 stack remain as recorded above. Current hosted checks are passing or skipped and this branch merges cleanly. #8890 remains the prerequisite. The separate landing-order coordination with the overlapping PR authors is still pending; I am not treating the hold withdrawal as resolving those integration items. |
What
Add regression coverage for removing the throwing
pthread_timedjoin_npdeclaration from libkernel_sys so scoped imports reach the canonical implementation from #8890 through the existing kernel-variant fallback merged in #8867. Keep requested-module precedence unchanged. The public PS5 3.20 tables list NIDPkS44IGrDkMin libkernel, libkernel_sys and libkernel_web.The added Windows regressions relink synthetic ELF executables with real NID-scoped imports and load the actual patched libraries. They check null arguments, invalid-deadline precedence before an invalid thread pointer, and a guest thread created and timed-joined through the same requested variant, including its full-width return value. libkernel_web already reaches the canonical implementation and remains a passing control.
Depends on #8890. This follow-up is stacked on that PR plus main
6ef5a1c4c92272598b18adeb9b5414112f8d15ed; its existing prerequisite changes remain visible in the aggregate diff until #8890 merges. The focused follow-up commit is6cf0937ccf. The stub removal is now also included in the refreshed prerequisite to satisfy the current duplicate-export rule; the remaining focused diff adds regression coverage; it does not introduce duplicate providers or a new timed-join implementation. The behavior and console-equivalence limitations in #8890 remain applicable.Tested
Windows x86-64, AMD Ryzen 7 9800X3D, WinLibs MinGW-w64 GCC 15.2, Release, full build configuration with BUILD_TESTING=ON and bundled dependencies. No games or PS5 hardware run.
Regression before implementation: the sys-scoped fixture aborts at the existing throwing stub; web-scoped validation and lifecycle cases pass. After the deletion, both variants pass. The final identical generated executables, swapping only the requested variant DLL, give four sys failures and four web passes on baseline, then eight passes on candidate. The unchanged candidate executables also pass ten repetitions each (80 executions). The shared canonical timed-join implementation is present in both comparisons.
Independent source review found no actionable issue; it did not independently run the Windows tests. Conventions, whitespace and test registration pass (1,168 test executables, none unregistered). Linux/macOS and console behavior are not measured here. The full Windows libs build succeeds; all 20 selected suites pass with normal Windows filesystem permissions, including both new import regressions, canonical timed-join/lifecycle/cancellation controls, guest_kernel_sys and the 360-library loader (zero load failures). The first sandboxed run passed 19/20: guest_kernel_sys aborted creating a hard link with Access denied; the same baseline executable and the candidate pass outside that restriction. Both filesystem runs skip symbolic-link cases because the host cannot create them. The full repository CTest suite was not run.
The required overlap scan found no compared-file/export conflicts. Its 22 base-conflicted shared-file warnings were reviewed against current PR refs. #6044 reorganizes test paths/registration; #7149 changes the native join tail and remains the prerequisite implementation's integration concern. Neither duplicates the new actual-library scoped-import regression; #9000 tests general provider precedence with synthetic providers. #8890 is the declared prerequisite, not a duplicate resubmission.
Review follow-up
Merged current main
329efe1bfdaff116c57cdf62718026ee4f5521aewithout rewriting published history. Canonical #8890 head:235ea5fd2260c6327eab43a82f08fd70c2236925; dependent #9582 head:da52d09106ac0353b8b0d9f4de526d1c1b4caeb7. The shared Thread.cpp is identical between these heads. The refreshed combined stack rebuilt affected Windows libraries and passed all 20 selected timed-join, sys/web import, lifecycle/cancellation, kernel-sys and patched-library suites. This is a combined-stack run, not a separate run of the prerequisite-only DLL. Linux/macOS and console execution remain unavailable locally. Test registration and conventions were rechecked. The refreshed checker rejects #8890 alone while the duplicate sys discovery declaration exists, so its deletion is now included in #8890 (235ea5f). #9582 retains the actual-library sys/web regression coverage. The final combined tree is byte-for-byte identical to tested stack 4158a67; this history adjustment changes no tested source.Proposed integration order is #8890 followed by #9582. Coordination with #7149 must retain its successful Windows join stack reaping in the shared join-success path and its detach/exit behavior. #9713 must retain its five sys implementations while dropping their discovery declarations and this follow-up's timed-join declaration. If #6044 lands first, move/register the new tests according to its final directory/helper/label conventions. These external landing agreements are pending; the unrelated branches were not combined or tested here. There is no duplicate timed-join provider.
The latest overlap scan (upstream snapshot b7d1d47) reports textual conflicts with #9713 and #9770, plus 22 base-conflicted shared-file warnings. Manual inspection also identifies #4563: its Reap-before-native-wait protocol must be retained inside the shared success path if combined, with AwaitReap/detach handling preserved. #9770 removes/reorganizes the adjacent registration block; keep the timed-join wrapper and verify registration after either landing order. No combined validation of these external branches is claimed. These coordination items remain open rather than being presented as clean integration.
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)