Repository navigation
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
There was a problem hiding this comment.
Reviewed the current draft head 0d6a3fe10617cfff2bc3164f905521785eaf73e5. The checked copy precedes all KernelTimespec field reads, restores host errno, and returns the existing guest EFAULT mapping; the boundary cases cover inaccessible and split-page requests across all three exports. I found no source-level blocker in this diff.
I independently configured a fresh Linux Release build and built both libkernel and guest_nanosleep_tests successfully. The focused guest_nanosleep CTest aborts during HostCpu static initialization, before reaching assertions, because this Xeon E5-1620 v2 lacks the PS5-required LZCNT/BMI/AVX2/FMA features. This is not a passing runtime test; the macOS component-harness results in the description remain the available runtime evidence. The branch is 261 commits behind current main but merges cleanly. Please update/revalidate the integration on a supported host before marking the draft ready.
0d6a3fe to
c95820f
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact rebased head c95820f4e4511fc0aa8ce507f73d434f4d675fa6 against main 74acfad65b3f0fd4eab653f3a3989d298fba1157. The authored patch is unchanged from the previously reviewed head (same stable patch ID); rebase introduces no additional code changes. The checked full-structure copy still precedes all timespec field reads, preserves host errno, and returns the existing POSIX/SCE EFAULT mapping. Boundary controls and all three exported aliases remain covered.
Validation evidence: the prior exact-code Linux Release build of libkernel and guest_nanosleep_tests succeeded, but this host's CTest run aborted before assertions during HostCpu initialization because required CPU features are unavailable. The author reports the rebased macOS component test, ASan/UBSan, x86_64 translation-unit compiles, conventions and whitespace checks pass; the component's adapters and limits are described in the PR. Hosted conventions, eligibility, labels and report pass; native product jobs are skipped by path policy. No full linked runtime or title run is claimed. No source blocker remains in this focused change.
jotitadev
left a comment
There was a problem hiding this comment.
First full native run of guest_nanosleep on Linux, which the Tested section asks for (the earlier Linux attempt aborted in HostCpu init on a CPU without AVX2/BMI).
Exact head c95820f4e4511fc0aa8ce507f73d434f4d675fa6, merged on main cb45ae2dd. Linux x86_64 (bare metal, not WSL), i7-8700, Ubuntu 26.04, kernel 7.0, GCC 15.2, glibc 2.43, CMake 4.2.3, Ninja, Release, BUILD_TESTING=ON, full build.
guest_nanosleeppasses, 3 of 3 runs, with the linked executable and the reallibkernel.- The existing sleep/time tests pass, 16 of 16:
guest_nanosleep,guest_nanosleep_long_request,guest_sleep_wake, the 8guest_sleep_cancel_*, the 2guest_timed_join_*,guest_freebsd_clocks,guest_time_conversion,guest_time_extras. - Full non-GPU suite on the same
mainwithout the PR: 1361/1361. - With only
core/libs/prx/libkernel/Time/Time.cppreverted tomain(new test kept),guest_nanosleepcrashes with SIGSEGV. So the unreadable-request case also crashes on Linux, and the test catches it there.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Windows run of this head
AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Follow-up after comparing the reviewed head with current upstream main 801d5b9bc1: this PR still targets base 74acfad65b, and core/libs/prx/libkernel/Time/Time.cpp has since changed. Current main now contains a checked ReadGuestTime helper in that same file, used by other time APIs, while this PR adds SocketRuntime.hpp and a separate GuestSocketReadGuest call in SleepForRequest. Please rebase and integrate the full-request snapshot with the current helper/path, then rerun the focused linked guest_nanosleep test and conventions on the merged tree. The independent Linux 3/3 + 16/16 result at the submitted head is valuable, but does not validate this current-main integration. The earlier approval is superseded until that recheck.
c95820f to
7af3272
Compare
What
Copy the complete guest
KernelTimespecthrough the current Time.cppReadGuestTimehelper before validating fields or starting a sleep. A stable inaccessible page now returns EFAULT throughnanosleepand_nanosleep, or SCE_KERNEL_ERROR_EFAULT throughsceKernelNanosleep, instead of faulting the host. Preserve errno across the copy; the POSIX wrappers set their error as before.Extend
GuestNanosleepwith protected-page cases for all three exports: wholly inaccessible requests, a readable first field with an inaccessible second field, readable same-address controls, complete requests immediately before the protected page, restored permissions, and unchanged remaining-time output on failure. Existing duration/invalid-value/alias checks remain.The request snapshot preserves the existing validation and sleep behavior. This revision rebases onto main
801d5b9bc11049e6274daca09d70690bd5b8d0a9and reuses its checkedReadGuestTimehelper; the SocketRuntime include and GuestSocketReadGuest call are removed.Overlap: #8916 changes cancellation policy, while this change snapshots the input before validation. Prior source/component composition was checked at its head a80a247; that historical check is not a runtime validation of this revision. The manually inspected clock-setter changes in #11042 and #11476 and clock-source change in #9677 affect different functions. No dependency is required.
Tested
Current head
7af32724b934054e17db210ffa2d63f23511be31, on main801d5b9bc11049e6274daca09d70690bd5b8d0a9:Pending: the focused linked
guest_nanosleepruntime test on this new current-main integration. No supported Linux/Windows runtime host is available locally. The previous approval was superseded by the review requesting this integration and retest.Historical external Linux evidence: jotitadev tested previous head c95820f composed with main cb45ae2 on i7-8700/Ubuntu26.04/GCC15.2 Release. Linked guest_nanosleep passed 3/3 and focused sleep/time 16/16; main Time.cpp with the new test crashed. Those results do not validate the changed helper integration in this head. The reported 1361/1361 suite belonged to main only; combined SHA/raw logs were not supplied.
Not tested on this head: linked native Linux/Windows, real guest/title execution, or hosted runtime CI. No proprietary modules used.
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)