Repository navigation
Conversation
github-actions
Bot
added
the
unblocks title
|
Exports with a name or library/NID already present in
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The fix is supported by the reported Linux testing, but overlapping open changes need an agreed landing order and two title-unblock entries lack their required discussion references.
What I ran
Reviewed head fe58f461d6a5e7685333adab1ad6c347b914933b against PR base e2df06e661f94d6e63b8f6622c8c58507b0245c1. The live merge preview onto main 6475267b456de6a0a23b230aa751f0b917d12688 was clean. Environment: Debian Linux x86_64, Python 3.11.2, GCC 12.2.0. I did not build locally or run CTest.
git diff --check e2df06e661f94d6e63b8f6622c8c58507b0245c1...fe58f461d6a5e7685333adab1ad6c347b914933b
python3 tools/check_conventions.py --base e2df06e661f94d6e63b8f6622c8c58507b0245c1 --head fe58f461d6a5e7685333adab1ad6c347b914933b --title 'fix(prx): keep forwarding exports from binding to themselves on Linux' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13730-fe58f461d6a5/pr-body.md
python3 tools/pr_stack_check.py 13730 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13730-fe58f461d6a5/stack
python3 tools/pr_abi_audit.py --base e2df06e661f94d6e63b8f6622c8c58507b0245c1 --head refs/pr/13730 --json
python3 tools/pr_test_quality.py --root /tmp/pr-13730-quality --json core/libs/tests/PatchedSelfImports.cpp
python3 tools/pr_overlap.py --branch refs/pr/13730 --repo boykopovar/AnyPS5 --jobs 4
gh pr checks 13730 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff check | Passed. |
| Conventions and PR template | Passed. |
| Merge preview | Clean on current main at 6475267b456de6a0a23b230aa751f0b917d12688. |
| ABI audit | passed_with_warnings; the sceAmprWcstombsS alias warning concerns its explicit NID, while this PR changes only its call body. |
| Test-quality heuristic and registration | Heuristic flagged the C++ test as possibly unregistered; this is a false positive because the changed CMake file registers the target and CTest entry. |
Hosted conventions |
Passed. |
Hosted eligibility |
Passed. |
Hosted intake |
Passed. |
Hosted labels |
Passed. |
Hosted library-portability |
Passed. |
| Hosted register-context Ubuntu Debug | Passed. |
| Hosted register-context Ubuntu Release | Passed. |
| Hosted relinker Ubuntu | Passed. |
| Hosted relinker Windows | Passed. |
| Hosted relinker ASAN | Passed. |
Hosted report |
Passed. |
Hosted linux |
Skipped. |
| Hosted register-context macOS Debug | Pending. |
| Hosted register-context macOS Release | Pending. |
Hosted relinker macos-latest, clang++ |
Pending. |
| Hosted relinker macOS x86_64 | Pending. |
| Hosted write-watch macOS arm64 | Pending. |
| Hosted write-watch macOS Intel | Pending. |
Hosted full windows job |
Passed on latest refresh. |
| Local build and CTest | Not run. |
Unedited output excerpts
check_conventions.py:
All conventions checks passed.
The test-quality helper output for the new C++ test:
{
"findings": [
{
"path": "core/libs/tests/PatchedSelfImports.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."
]
}Relevant overlap-scan rows (the scan used current main 6475267b456de6a0a23b230aa751f0b917d12688):
| [PR 7645](https://www.github.com/boykopovar/AnyPS5/pull/7645) | `core/libs/CMakeLists.txt` [L343-344](https://github.com/boykopovar/AnyPS5/blob/6475267b456de6a0a23b230aa751f0b917d12688/core/libs/CMakeLists.txt#L343-L344) | | |
| [PR 9019](https://www.github.com/boykopovar/AnyPS5/pull/9019) | `core/libs/prx/libkernel/File/src/Stdio.cpp` [L1085](https://github.com/boykopovar/AnyPS5/blob/6475267b456de6a0a23b230aa751f0b917d12688/core/libs/prx/libkernel/File/src/Stdio.cpp#L1085) | `pwrite` | |
| [PR 12868](https://www.github.com/boykopovar/AnyPS5/pull/12868) | `core/libs/prx/libkernel_sys/Aio.cpp` [L96-98](https://github.com/boykopovar/AnyPS5/blob/6475267b456de6a0a23b230aa751f0b917d12688/core/libs/prx/libkernel_sys/Aio.cpp#L96-L98) | | |
Manual patch inspection found #7645 changes a different CMake hunk, so it is not a substantive overlap here. The scanner could not compare several branches that conflict with the base; this PR's own description also identifies #12700 and #12965 as touching the positioned-write change.
Latest relevant gh pr checks output:
relinker (ubuntu-latest, g++) pass 2m42s https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227358
relinker (windows-latest, g++) pass 9m31s https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227403
relinker-asan pass 2m36s https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227374
register-context (ubuntu-latest, Debug) pass 30s https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227340
register-context (ubuntu-latest, Release) pass 26s https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227343
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423228639
register-context (macos-15-intel, Debug) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227369
register-context (macos-15-intel, Release) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227401
relinker (macos-latest, clang++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227502
relinker (macos-latest, clang++, x86_64) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227509
windows pass 21m55s https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227351
write-watch (macos-15) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227368
write-watch (macos-15-intel) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41162362960/job/124423227336
Negative control
| Assertion | Change to code under test | Did the test fail? |
|---|---|---|
| Self-import regression must be detected | No negative-control mutation or exact-head test binary was available without building. | Not exercised. |
Findings
Blocking
CONTRIBUTING.mdrequires authors of overlapping PRs to agree on landing order and record it withDepends on. The live overlap scan found open #9019 changing thepwriteprovider path and #12868 changing the AIO call. This PR's description also names #12700 and #12965 as touching the positioned-write code. The checklist currently saysDepends on: none, and no coordination is recorded. Please agree on the landing order with the affected authors and update the checklist accordingly.- The
Unblocksline listsPPSA98514andPPSA20040without their Game bring-up discussion references. Add the applicable discussion IDs or remove entries that are not tracked there.
Non-blocking
The description reports substantial Linux and title testing but does not state the exact PR head SHA those results exercised. Please add the full tested head SHA so the evidence can be matched to this revision.
Sources
- Overlap-order rule at current
main - Required title and discussion format at current
main - PR #13730 test registration at reviewed head
- PR #13730 positioned-write provider at reviewed head
- PR #13730 AIO call at reviewed head
- PR #9019 positioned-write provider at its current head
- PR #12868 retained-file AIO path at its current head
- PR #12700 positioned-write changes at its current head
- PR #12965 positioned-write changes at its current head
Not verified
No local compilation, CTest, title launch, console behavior, or negative-control mutation was run. The Linux-specific test is not covered by the hosted Linux job because that job was skipped; macOS and full Windows jobs were pending. The exact PR head used for the author's reported tests is unspecified. The overlap scan could not compare some branches that conflict with the PR base, so they still need manual review before concluding no additional overlap exists.
To get this to a good state
Please coordinate and record the overlap order, complete the missing Game bring-up references, and add the exact tested head SHA. I will rerun the relevant checks on the updated head.
Tests run and reviewed — status correctionThe prior review’s “Not verified” section incorrectly marked the full Windows job as pending; the latest check shows it passed. What I ranChecked PR head gh pr checks 13730 --repo boykopovar/AnyPS5Results
Unedited outputNot verifiedThe macOS jobs have not completed; no local build or CTest was run. |
fe58f46 to
e9e816f
Compare
|
@MarcZX10 thanks, all three are in the description now, at head
|
|
The two macOS |
|
Tested
They come from #14240 (merged 2026-11-01). Setup: WSL2 Ubuntu 22.04, GCC 11.4.0, Ninja, Release, Tests:
The two macOS AI-assisted: yes (Claude Code) |
libScePosixForWebKit forwards opendir, closedir, readdir and readdir_r to libc's functions of the same NIDs, libSceAmpr forwards sceAmprWcstombsS to libc's wcstombs_s, libSceLibcInternal forwards its aligned nothrow new[] and aligned delete[] to libc's, and libkernel_sys's AIO writes call libkernel's pwrite, whose NID libkernel_sys also exports. After NID patching such an import has the name of the library's own export, and when that library comes before the provider in the lookup scope, the import binds to the export: the WebKit, Ampr and LibcInternal forwarders call themselves forever, and an AIO write reaches libkernel_sys's unimplemented pwrite. The providers now keep each body in a _nid_no_patch function that the export and the forwarders call. patched_self_imports checks on Linux that no patched library imports a name it exports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e9e816f to
9056177
Compare
|
@B-Howell thanks, confirmed: #14240's Head
|
XairRrR
left a comment
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
Moving the eight self-forwarded bodies into _nid_no_patch helpers removes every self-import from the patched libraries on Linux. The new patched_self_imports test passes on the merged tree, and with main's production files it fails and lists exactly those eight names.
What I ran
Head 905617771ee037b6855f27750d9d8480a4e851a4, merged with --no-ff into main 9090972b7e91fd266819cbd77a8b058e177b5682 (merged tree a200042f4212607ed244e8a51a9c4b5ca8ee051e). Debian 13 VM, 32 vCPU Xeon Gold 6136, GCC 14.2.0, Release, Ninja, BUILD_TESTING=ON, lavapipe (Mesa 26.1.6).
git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...905617771ee037b6855f27750d9d8480a4e851a4
python3 tools/check_conventions.py --base 9090972b7e91fd266819cbd77a8b058e177b5682
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "patched_self_imports|guest_directory|guest_opendir2|guest_fdopendir|guest_posix_filesystem|guest_posix_wrap|guest_kernel_sys_aio|guest_aio|guest_ampr|guest_wcsrtombs|guest_libc_internal_aligned_new|guest_new_handler|guest_kernel_sys_pwrite"
# negative control: the 11 production files restored from main
git show 9090972b7e91fd266819cbd77a8b058e177b5682:<path> > <path>
ninja -C build -j32 -l32 libs patched_self_imports_tests
ctest --test-dir build --output-on-failure -R "^patched_self_imports$"
# restore the merged files
git show a200042f4212607ed244e8a51a9c4b5ca8ee051e:<path> > <path>
ninja -C build -j32 -l32 libs patched_self_imports_tests
ctest --test-dir build -V -R "^patched_self_imports$"
git status --porcelainResults
| Check | Result |
|---|---|
git diff --check |
Clean |
check_conventions.py |
All conventions checks passed |
Configure, build, libs (script) |
OK |
| Script test run (3608 tests) | No new failures and no timeouts. The 9 known failures also fail on main. The unexpected guest_net_ctl skip also happens on main. |
| Focused tests (25, listed above) | 25/25 pass, the longest is patched_self_imports at 0.99 s |
patched_self_imports on the merged tree |
No patched library imports a name it exports (405 libraries) |
| Negative control | Fails with the eight names from the description |
| Worktree after restoring | git status --porcelain is empty |
| Validation-layer comparison | Not applicable: the change touches no AGC driver, shader recompiler or VideoOut file |
Negative control output
1/1 Test #410: patched_self_imports .............***Failed 0.91 sec
libSceAmpr.prx imports sZLrjx-yEx4, a name it also exports, so the import can bind to its own export
libkernel_sys.prx imports C2kJ-byS5rM, a name it also exports, so the import can bind to its own export
libScePosixForWebKit.prx imports J0kng1yac3M, a name it also exports, so the import can bind to its own export
libScePosixForWebKit.prx imports XepdqehVYe4, a name it also exports, so the import can bind to its own export
libScePosixForWebKit.prx imports ay3uROQAc5A, a name it also exports, so the import can bind to its own export
libScePosixForWebKit.prx imports lybyyKtP54c, a name it also exports, so the import can bind to its own export
libSceLibcInternal.prx imports s2eGsgUF9vk, a name it also exports, so the import can bind to its own export
libSceLibcInternal.prx imports v09ZcAhZzSc, a name it also exports, so the import can bind to its own export
Negative control
I restored all 11 non-test files the PR changes from main. The test registration in core/libs/CMakeLists.txt and PatchedSelfImports.cpp stayed as merged. After rebuilding libs, patched_self_imports failed and reported the same eight self-imports the description lists. These include the two libSceLibcInternal ones that B-Howell found in their comment. I then restored the merged files and rebuilt, and the test passed again (405 libraries). The worktree matched the merged tree at the end.
Findings
Blocking
None.
Non-blocking
- The
Depends online doesn't match the author's comment. In the 2026-10-31 reply to MarcZX10, the author wroteDepends on: #9019, #12868, and said this PR would rebase onto #12868'sAio.cppstructure once that PR lands. The current checklist names only #9019. The new call atcore/libs/prx/libkernel_sys/Aio.cpp:98is the line #12868 restructures, according to MarcZX10's review. Either restore #12868 on the line or say on the PR that the order with #12868 no longer matters. This keeps the landing-order record CONTRIBUTING asks for consistent. - The new helpers use the default ABI.
WideToMultibyteChecked_nid_no_patch(core/libs/prx/libc/src/Multibyte.cpp:372),AlignedArrayNewNothrow_nid_no_patchandAlignedArrayDelete_nid_no_patch(core/libs/prx/libc/src/StdioExtras.cpp:171,:191) andPosixPwrite_nid_no_patch(core/libs/prx/libkernel/File/src/Stdio.cpp:1117) have noAPS5_VABI. The directory helpers do have it. All callers are host code, and each declaration matches its definition, so this is correct. It is only noted as an inconsistency between the helpers.
Not verified
- No title was run; the hang fix for Apotris, FFTARecomp and the other titles rests on the author's report.
- The Windows and macOS builds of this head were not built here. MarcZX10's review reports the hosted Windows job passing on an earlier head,
fe58f461d6. - I could not check that the PR merges with #9019 without conflict, which the author reports from
git merge-tree. That needs the other PR's head, and the sandbox has no network. - GPU tests on vendors other than lavapipe were not run; the change touches no GPU path.
|
@XairRrR thanks for the run and the negative control.
|
What
Four libraries forward an export to another library's function of the same NID:
opendir,closedir,readdir,readdir_r(#12386,*_nid_disambig1_nid_postfix)opendir_nid_postfix…sceAmprWcstombsS(sZLrjx-yEx4)wcstombs_s_nid_postfix(sZLrjx-yEx4)Aio.cpp(and its own unimplementedpwrite_nid_postfix,C2kJ-byS5rM)pwrite_nid_disambig1_nid_postfix(C2kJ-byS5rM)new[]nothrow (s2eGsgUF9vk) and aligneddelete[](v09ZcAhZzSc) inAlignedNew.cpp(#14240)_ZnamSt11align_val_tRKSt9nothrow_t_nid_postfixand_ZdaPvSt11align_val_t_nid_postfix(same NIDs)After NID patching the import and the library's own export have the same name, for example
ay3uROQAc5Ain libScePosixForWebKit (nm -D:U ay3uROQAc5AandT ay3uROQAc5A). On Linux the import binds to the first definition in the lookup scope, and when the library comes before the provider it binds to its own export. libScePosixForWebKit'sopendirisjmp ay3uROQAc5A@plt, so it jumps to itself forever: Apotris (PPSA98513), FFTARecomp (PPSA98512) and DevilutionX (PPSA98514) hang at start at about 95% of one core with no system calls, and gdb samples are always inopendir_nid_disambig1_nid_postfixor that PLT entry. WithLD_PRELOADof the patchedlibc.prx, which puts libc first in the scope, Apotris starts.sceAmprWcstombsSand libSceLibcInternal's two aligned forwarders can loop the same way, and an AIO write can reach libkernel_sys'sNotImplementedpwrite. PE and Mach-O imports name their library, so Windows and macOS don't have this.Each provider now keeps the body in a
_nid_no_patchfunction (OpenDirectory,ReadDirectory,ReadDirectoryEntry,CloseDirectory,WideToMultibyteChecked,AlignedArrayNewNothrow,AlignedArrayDelete, and #9019'sPosixPwrite), its export calls that function, and the forwarders call it too. The body has to move, because a_nid_no_patchwrapper that called the exported name could itself be interposed. Nothing changes in what the functions do.patched_self_imports(Linux, depends onlibs, likewindows_import_identity) reads the dynamic symbols of every.prxincore/libs/libsand fails for an import whose name the same library exports. Onmainit reports the eight names above:With this PR:
No patched library imports a name it exports (405 libraries).Landing order (CONTRIBUTING: overlapping PRs agree an order): this PR goes after #9019 (
Depends onbelow), so #9019 doesn't have to change anything.pwriteaPosixPwrite_nid_no_patchhelper. This PR carries that hunk verbatim inStdio.cppandFile.hpp(git merge-treeon both heads: no conflict), so once feat(libkernel_sys): implement pwrite through the shared POSIX provider #9019 is in, those lines are no change here; this PR only calls the helper from libkernel_sys's AIO write.pwrite's body for cancellation, which meets feat(libkernel_sys): implement pwrite through the shared POSIX provider #9019's hunk, not a line of this PR's own.Tested
Linux Mint 22.3, kernel 7.0.0-38, GCC 13.3.0, Release,
BUILD_TESTING=ON, head905617771eonmain784ab09683(merges cleanly with currentmain). The titles ran on earlier heads, before the rebase that added the two libSceLibcInternal forwarders: the first headfe58f461d6, which differs only in the name of the pwrite helper and where the test is registered, ande9e816f6c7for I Wanna Be The Guy.Catalog titles, converted with
--to-intel, 60 s each under Xvfb on an NVIDIA GTX 1050 Ti (580.178.04):mainopendiropendir(gdb)e9e816f6c7)All tests once and every GPU test on each of the three devices, under Xvfb, on
905617771e: NVIDIA GTX 1050 Ti 3596/3596 run, 1 failed (agc_driver_htile_mask_lifetime, "other device survives cleanup", which fails the same way onmain784ab09683on this GPU, 9 of 10 runs alone, and passes on the other two devices); Intel HD Graphics 630 (Mesa 25.2.8) 898 run, 2 failed (agc_driver_f64_rounding_denormal_modes,agc_idle_publish_wait), the same two asmain784ab09683on this GPU; llvmpipe 898 run, 0 failed.check_skips.pyreports no skip thatmaindoesn't.patched_self_imports, the directory, multibyte, POSIX-for-WebKit, AIO, Ampr and alignednew/deletetests (guest_directory*,guest_opendir2,guest_fdopendir,guest_posix_filesystem,guest_posix_wrap,guest_kernel_sys_aio,guest_aio*,guest_ampr*,guest_wcsrtombs*,guest_multibyte_locale,guest_working_directory,guest_libc_internal_aligned_new,guest_new_handler*,nid_*): 39/39 pass.check_conventions.py,check_test_registration.py,check_main_sync.pyandcheck_compile.pyon the changed files are clean.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