Repository navigation
feat(libkernel): export the POSIX rename - #1192
Conversation
Dextroid17
left a comment
There was a problem hiding this comment.
Built and ran it — and then found that the existing test for this symbol cannot tell whether your change is there, which is worth knowing before this is relied on as covered.
Linux (CachyOS, GCC, Release):
cmake --build build --target guest_filesystem_tests --parallel 16
ctest -R ^guest_filesystem$ → 1/1 Passed
The suite passes with and without the change. guest_filesystem_tests links both libkernel and libc, and libc.prx already exports this symbol on your base — libc/src/Filesystem.cpp:93:
extern "C" int APS5_VABI rename_nid_postfix(const char* from, const char* to) {
if (!from || !to) { errno = 14; return -1; }
...
std::filesystem::rename(source, destination, error);nm on the test binary shows the symbol as U rename_nid_postfix — undefined in the executable and bound at load time from one of the two libraries (ldd lists both libkernel.prx and libc.prx). Reverting only your Stdio.cpp hunk and rebuilding:
pr-1192 (libkernel defines it): 100% passed
pr-1192^ (only libc defines it): 100% passed
So the test binds to whichever library the loader picks and passes either way; it exercises a rename, not the one you added. The existing assertions (rename to a new name, to itself, to a missing source, back again) are real coverage of the function — just not of this implementation.
The thing I would ask about is the second implementation. There are now two definitions of one POSIX function, and they do not follow the same path:
- libc: rejects a null argument as
EFAULT(14) and an empty one asENOENT(2), thenResolvePath+std::filesystem::renamewithFilesystemErrormapping. - libkernel (yours):
PathErroron both arguments, thensceKernelRenamethroughPosixResult.
Both are defensible, and each is internal-consistent — but a guest that imports rename from libkernel and one that imports it from libc now run different validation and different errno mapping, and two implementations of one function drift as soon as either is edited. If a title or a NID needs rename exported from libkernel specifically, putting that in the body makes the duplication obviously intentional; if not, one of the two delegating to the other would keep a single behaviour.
Nothing blocking — the build and the suite are clean, and adding the export is not harmful. It is the claim of coverage I wanted to correct, because the passing test looks like it covers this and does not.
This is a comment, not an approval — I have no merge rights here.
Written with AI assistance.
|
Thanks for the careful check, and you're right: On the duplicate: the export has to exist in libkernel because Silksong ships its own |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks. Built and ran this merged onto current main on Linux: everything passes except the new test.
core/libs/tests/GuestKernelRename.cpp:36:guest_kernel_renameaborts on Linux at the first rename (Kernel rename check failed at line 42 (guest errno 2)).rootis built fromstd::filesystem::temp_directory_path(), an absolute host path, and the guest VFS on Linux doesn't map those.GuestFilesystem.cpp:89uses a relative directory (std::filesystem::path("anyps5-filesystem-test-" + ...)) for this reason; ctest runs in the build dir. Withrootbuilt the same way the test passes here. Please change it, so the Linux CI job doesn't go red.
The export itself looks fine: it matches the other POSIX wrappers in Stdio.cpp, and since the title imports rename from libkernel rather than libc, having it there is intentional, not a duplicate to remove.
Not verified on our side: Windows (the LoadLibraryA path and the Silksong import).
|
Thanks @adriadam10, good catch. Fixed in 5b84912: the test now builds its directory relative to the working directory, the same way I also merged current On Windows, the |
adriadam10
left a comment
There was a problem hiding this comment.
Checked on Linux, merged onto current main: build, libs and full ctest green (434/434), and the relative-directory fix in 5b84912 works. Two things before merging:
- The conventions check fails:
[duplicate-export] rename_nid_postfix is also in libc(Stdio.cpp:648). The project rule (#228, #241) is one implementation per export. Since the title ships its ownlibc.prxand only loads libkernel'srename, this is the case the shared-source pattern is for (FontLibrary.cmake): keep a singlerenamedefinition in one source compiled into both libraries, instead of a second copy inStdio.cpp. - On Linux the new test also passes with the export removed:
dlsymon the libkernel handle also searches its dependencies, andlibcexports the symbol. Check withdladdrthat the symbol comes fromlibkernel(we have a version of this ready: it fails without the export and passes with it), so the test pins the export.
Not verified on our side: Windows (LoadLibraryA path, the Silksong import).
5b84912 to
76db9fa
Compare
|
Thanks @adriadam10, both items are done in 76db9fa, rebased onto current
On Windows, |
|
Verified the current head on Linux, and both points @adriadam10 left hold here. Linux (CachyOS, GCC, Release), head The test now pins the libkernel export, which is the thing I could not get out of the old one. With the single line that compiles Restored, it passes again and One implementation: Nothing else from me on the test: the coverage claim I raised is now backed by a control rather than by a passing run. Not verified here: Windows (the Written with AI assistance. |
|
Status on the current head, since both of your points are now done and the remaining blocker is not in the code. Linux, head The thing actually holding the merge: no CI has run on this head. The workflow run for So @adriadam10 — if you approve the run for this head (and the other two), CI becomes the check rather than a missing signal, and the Windows Written with AI assistance. |
|
The full list is now in one place: #1631 — 38 open PR heads queued at Written with AI assistance. |
Titles that ship their own libc in sce_module import rename from libkernel (NID NN01qLRhiqU), where it was missing, so PPSA12544 failed to start. It checks both paths like unlink and maps sceKernelRename's result to errno. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
guest_filesystem_tests links libkernel and libc, both of which define rename_nid_postfix, so which one it binds to depends on the loader and the suite passes either way. Resolve the symbol from libkernel.prx explicitly so the export added for titles that ship their own libc.prx is actually tested. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The guest VFS on Linux does not map absolute host paths, so the test failed at the first rename there. Match GuestFilesystem.cpp and build the directory relative to the working directory. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The repo allows one implementation per export. Move libc's rename into libc/src/Rename.cpp, compile that file into both libc and libkernel, and drop the second copy from libkernel's Stdio.cpp. FilesystemError moves to a header so the shared source can use it. The test now checks that the symbol it resolves belongs to libkernel (module handle on Windows, dladdr on Linux) so it fails when the export is removed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
76db9fa to
1bd0ef8
Compare
|
Rebased onto current |
|
Rebase noted. The question from my review is still the one that decides this, and it is the same one the maintainer puts to other system-library changes: which title imports On The rebase itself looks clean from here. Written with AI assistance. |
adriadam10
left a comment
There was a problem hiding this comment.
Checked on Linux merged onto current main: build, libs and ctest green apart from patched_libraries, which depends on directory iteration order (libs without RUNPATH needing libkernel.prx) and also fails on unrelated branches. guest_kernel_rename passes, and fails with "rename_nid_postfix is not defined by libkernel" when Rename.cpp is dropped from libkernel's sources. libc and libkernel now share one rename (libc/src/Rename.cpp), so the earlier concern about two implementations drifting no longer applies, and the PR body names the title (PPSA12544) and the unresolved-import log line. Merging once CI is green.
|
To close the loop on the question: PPSA12544 (Hollow Knight: Silksong) imports Written with AI assistance. |
What
PPSA12544 (Hollow Knight: Silksong, v1.0.30021) imports
rename(NIDNN01qLRhiqU) from libkernel, not from libc. The title ships its ownlibc.prxinsce_module, so the host libc'srename(added in 91bf8e3) is not in the lookup. libkernel did not export it, and with--windows-diagnosticsstartup stopped with:renamenext tounlink. Like the other POSIX wrappers inStdio.cpp, it checks both paths withPathError(null ?EFAULT, empty ?ENOENT) and maps thesceKernelRenameresult toerrnothroughPosixResult.guest_filesystem_testslinks bothlibkernelandlibc, which each definerename_nid_postfix, so which one it binds to depends on the loader (reviewer on Linux showed it passes with or without this change). The newguest_kernel_rename_testsresolves the symbol fromlibkernel.prxexplicitly and checks rename, rename onto itself, missing source (ENOENT) and null/empty arguments (EFAULT/ENOENT). It fails without the export. Windows only so far; thedlopenpath for Linux is untested.renameto libkernel.Tested
-DBUILD_TESTING=ON, full build,--target libs,ctest -R guest_: 131/131 pass.--windows --to-intel --module-dir Media/Plugins: with this change theNN01qLRhiqUimport resolves. On a local integration branch that also carries @merwie's Silksong PRs (feat(relinker): relink needed modules outside the module directories #716/feat(relinker): add --module-dir for modules loaded at run time #846, feat(libkernel): deliver sceKernelRaiseException on Windows #760, fix(appcontent): report no download data space without a declared area #842, feat(agc): implement sceAgcDriverGetEqContextId #844, fix(libkernel): report guest unwind tables in sceKernelGetModuleInfoForUnwind #904) and fix(agc): store and read DCC keys over the console's metadata extent #788/fix(agc): store uncompressed DCC keys at the first write of a fast-cleared image #903, the title gets past the Team Cherry logos to its main menu (Start game / Options / Extras) and holds a steady 60 FPS (vsync-locked) for 5+ minutes. Screenshot attached.0x00, key 0 is0xff, as in merwie's report on feat(agc): present DCC display buffers whose keys are uncompressed or fast-cleared #648), and two exports that open PRs already add (fix(libs): TLOU throwing stubs for VideoOut VrrPeg and VrrStatus #518 VRR peg/unpeg, feat(libs): Deathloop loading support (Ampr, NetCtlApDialog + stubs) #407sceNpEntitlementAccessGetEntitlementKey/sceNpUnregisterNpReachabilityStateCallback).renamefrom libkernel; I don't know whether merwie's version imports it.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)