Repository navigation
feat(libSceNKWebKitRequirements): implement the libjpeg, libxml2 and libpsl exports - #14421
PrivacyOnion wants to merge 14 commits into
Conversation
|
Please preserve each Keep the callback set associated with its registration and opened context, and leave existing registrations unchanged when a new registration fails. Please add regressions that actually parse through two distinct input handlers, continue using an earlier open context after another registration, and check failed-registration behavior. Cover the corresponding output-handler ownership as well. The current test only registers one dummy input and output set; it never calls them through I/O. |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The mirror layouts, submodule pins, CMake files and public-header audit check out, but the libxml2 I/O registrations share one set of guest callbacks, so a later registration changes the callbacks of earlier ones, and the debt document was rewritten with CRLF line endings, so git diff --check fails at ec1fa3c51e.
What I ran
Exact head ec1fa3c51ec88d663f13c3b97fc92b4fde802adb; PR base ba627802e2210187d417deb64c8f2fed8e33011f (current main), unchanged at completion. Stacking: the PR declares Depends on: #14374; the first three commits are #14374's (head 3979b705f3, an ancestor of this head), and the two new commits are 408a262b5e and ec1fa3c51e. No build and no test run (no-build review). Commands and tools:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421
python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head ec1fa3c51ec88d663f13c3b97fc92b4fde802adb --json
python3 core/libs/tests/test_public_headers.py --audit-only # run in an extracted copy of the head tree
git diff --check ba627802e2210187d417deb64c8f2fed8e33011f ec1fa3c51ec88d663f13c3b97fc92b4fde802adbHand checks: the LP64 layout of every sizeof/offsetof assertion in GuestJpeg.hpp, GuestXml.hpp and GuestPsl.hpp (a Python layout calculator over the header members, not a compile); the submodule pins against the upstream repositories; the new CMake files against the upstream CMake options; the export move between Export.cpp, Jpeg.cpp, Xml.cpp and Psl.cpp.
Results
| Check | Result |
|---|---|
| Diff check (precheck) | Failed: trailing whitespace on all 55 lines of docs/dev/debt/libSceNKWebKitRequirements.md (CRLF) |
| Conventions and PR template (precheck) | Passed; hosted conventions queued at this head |
| Live dependency and mergeability (precheck) | Passed |
Submodules .gitmodules and pins |
3rdparty/libxml2 is https://github.com/GNOME/libxml2.git at 7846b0a677f8 (tag v2.9.14, 2022-05-02). 3rdparty/libpsl is https://github.com/rockdaboot/libpsl.git at ba4c49a20540 (v0.21.5, 2024-01-13). Both commits exist upstream |
LP64 mirror asserts, GuestJpeg.hpp |
Computed layout matches all 5 sizeof and 120 offsetof assertions |
LP64 mirror asserts, GuestXml.hpp |
Computed layout matches all 8 sizeof and 172 offsetof assertions. The xmlDoc prefix offsets match libxml2 2.9.14 tree.h |
GuestPsl.hpp |
Opaque struct psl_ctx_st forward declaration; no layout to check |
CMake (cmake/LibXml2.cmake, cmake/LibPsl.cmake) |
All 38 LIBXML2_WITH_* options exist in upstream 2.9.14 CMakeLists.txt; the libpsl template variables and the src/psl-make-dafsa, src/psl.c, src/lookup_string_in_fixed_set.c sources exist at the pin |
| Public-header audit | 399 libraries, 26181 exports: unique declarations and umbrella coverage verified, exit 0 |
| ABI audit by hand | passed_with_warnings, 27 findings, all public-declaration-not-found: 5 static DefaultGuest* callbacks in Jpeg.cpp, 8 Dummy* callbacks, 5 Guest* callbacks and 9 Sax* callbacks, all static in tests/Jpeg.cpp and tests/Xml.cpp. None is an export |
| Export move | Export.cpp declarations go from 155 (3979b705f3) to 95 (head); exactly 60 names leave it and are defined in Jpeg.cpp (15), Xml.cpp (23) and Psl.cpp (22). The 95 left are 63 pending exports and 32 Unknown rows |
| Test registration | guest_nk_req_{jpeg,xml,psl} via foreach in CMakeLists.txt; guest_webkit_zlib from #14374 |
| Xml I/O registrations | Shared globals overwritten per registration (finding 1); the test registers one handler only |
| Hosted checks | Queued at this head |
Unedited diff check output (precheck)
docs/dev/debt/libSceNKWebKitRequirements.md:1: trailing whitespace.
+# libSceNKWebKitRequirements technical debt
docs/dev/debt/libSceNKWebKitRequirements.md:2: trailing whitespace.
+
docs/dev/debt/libSceNKWebKitRequirements.md:3: trailing whitespace.
Findings
Blocking:
- The libxml2 I/O registrations share one callback set.
xmlRegisterInputCallbacks_nid_postfixandxmlRegisterOutputCallbacks_nid_postfixoverwrite the globalss_guestInputMatch,s_guestInputOpen,s_guestInputRead,s_guestInputClose(and the output set) before calling libxml2 (Xml.cpp L695-L708 at head). Every host entry libxml2 stores for a registration uses the same trampoline, and the trampolines call the current globals (Xml.cpp L517-L521 at head). So registering a second handler changes the callbacks reached through the first handler's entry, including reads and closes for a context already opened through it. A registration that libxml2 rejects with-1has already overwritten the globals. The only test registers one dummy handler, so none of this is pinned. I confirmed the same point independently of the comment at 12:25 by oneandonlydean. Fix: keep each callback set with its registration and its opened contexts, leave existing registrations unchanged when a new one fails, and add regressions that parse through two distinct input handlers, keep using an earlier open context after another registration, and check a rejected registration; cover the output handler the same way. - The debt document uses CRLF line endings. Commit
408a262b5e(the first new commit) rewrotedocs/dev/debt/libSceNKWebKitRequirements.mdwith 55 CRLF lines; the stacked base3979b705f3has LF.git diff --checktherefore fails on every line, and the diff of that file is a whole-file rewrite that hides the real debt change. Fix: convert the file back to LF (for examplesed -i 's/\r$//') and push. Nothing else in the PR carries CR characters.
Non-blocking:
cmake/LibXml2.cmakesetsBUILD_SHARED_LIBSwithCACHE BOOL "" FORCE(line 6). The other bundled-library files use a plainset(for examplecore/libs/prx/libSceAgcDriver/CMakeLists.txtline 16). The forced cache write changes the value for every subdirectory configured after this one. Suggest the plainset.- The debt entry says the export names were taken from "libxml2 2.13.4" sources, but the bundled tree is libxml2 2.9.14. This is about provenance, not behaviour, but the two versions should be named the same way in the doc.
HostErrorExit(Jpeg.cpplines 39-41) throws a C++ exception through libjpeg's C frames. The debt entry discloses that errors become exceptions, but correctness relies on the unwind tables of the C code. Worth stating which toolchains this was checked on.- Several
Psl.cppassertions are weak:wildCount >= 0,excCount >= 0, a non-null check ondistFn, andint outdatedis discarded ((void)outdated). The version-number assertions are precise and match upstreampsl_check_version_number(returnsPSL_VERSION_NUMBERfor 0x001505 and 0 for 0x001000,src/psl.clines 1574-1590 atba4c49a2).
The debt counts check out against the code: 63 pending in Export.cpp, 32 Unknown table rows, 52 pending exports declared in Identified.hpp and 11 others, and 15 + 23 + 22 = 60 moved exports. The zlib claim (vendored 1.3.2 under libjpeg-turbo/src/spng/zlib with NO_GZIP defined in zconf.h) is accurate.
Sources
GuestXml.hppxmlDoc mirror against libxml2 2.9.14include/libxml/tree.h- libpsl
psl_check_version_numberat the pin - libjpeg-turbo vendored zlib
NO_GZIP cmake/LibXml2.cmakeat the head
Not verified
- Nothing was compiled or run; the layout result comes from a Python calculator over the header members and from reading the upstream structs, not from a build.
- The console's libjpeg, libxml2, libpsl and zlib versions and layouts are not verified (the debt says so).
- The "63 pending exports are present in PS5-3.20_Libs" claim was not checked against that inventory.
- Hosted checks at this head were queued; the Linux and Windows test results for the three new test binaries are not available.
To get this to a good state
Make the libxml2 I/O registrations keep their callbacks per registration, add the regressions described in finding 1, and convert docs/dev/debt/libSceNKWebKitRequirements.md to LF so that git diff --check ba627802e2210187d417deb64c8f2fed8e33011f HEAD is empty. The remaining points are optional. The diff check, the public-header audit, the layout checks and the Xml tests will be rerun on the new head.
|
@oneandonlydean thanks, fixed in fbc2ded. Ownership.
Regressions (
No export of this library opens an output buffer (there is no save or Against the previous Host harness (MinGW 13.1): the zlib, jpeg, xml and psl tests pass. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, I/O callbacks fixed; line endings still blocking
The libxml2 I/O registrations now keep one callback slot per registration, which fixes the shared-callback defect from my review at ec1fa3c51e, but the debt document still uses CRLF line endings, so git diff --check still fails at fbc2ded785.
What I ran
Exact head fbc2ded78536e77667752a3c29c0201457a53fc4; PR base ba627802e2210187d417deb64c8f2fed8e33011f (current main), unchanged at completion. The new commit touches only Xml.cpp and tests/Xml.cpp. No build and no test run. Commands:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421
python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head fbc2ded78536e77667752a3c29c0201457a53fc4 --json
python3 tools/pr_test_quality.py --root <git archive of fbc2ded785: core/libs/tests, core/libs/prx/libSceNKWebKitRequirements, core/libs/CMakeLists.txt> --json core/libs/tests/Xml.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cppI also read the slot table and the templated trampolines in Xml.cpp (L45-L46 and the Host*<Slot> functions), the registration functions (L745-L781), and the new regressions in tests/Xml.cpp (L292-L326 and L376-L414). The libxml2 callback table capacity at the pinned commit 7846b0a677f8 is MAX_INPUT_CALLBACK and MAX_OUTPUT_CALLBACK = 15 (xmlIO.c L100 and L117), which matches XmlInputSlotCount and XmlOutputSlotCount = 15.
Results
| Check | Result |
|---|---|
| Diff check (precheck) | Failed: 55 CRLF lines in docs/dev/debt/libSceNKWebKitRequirements.md (unchanged by this commit) |
| Conventions and PR template (precheck) | Passed |
| Live dependency and mergeability (precheck) | Passed |
| ABI audit (by hand) | passed_with_warnings, 27 findings, the same static test-helper set as before; none is an export |
| Test-quality heuristic (by hand) | Two possible-unregistered-cpp-test findings; false positives, because the Xml test is registered via foreach in libSceNKWebKitRequirements/CMakeLists.txt |
| I/O fix | Per-registration slots, rollback on a failed registration, and a full-table rejection; the tests cover two handlers, use of an earlier context after a later registration, a rejected registration and the output side |
| Build and tests | Not run (no-build review) |
| Hosted checks | Conventions queued at this head |
Findings
Blocking:
- The debt document still uses CRLF line endings: 55 lines from commit
408a262b5e, unchanged byfbc2ded785.git diff --checkfails on every line, and the file diff is a whole-file rewrite. Fix: convert the file to LF.
Non-blocking:
- A registration slot is never released, because libxml2 has no per-handler unregister. That is fine for the current use, but the debt entry should say so.
- The non-blocking points from my review at
ec1fa3c51e(the globalBUILD_SHARED_LIBSforce incmake/LibXml2.cmake, the libxml2 version wording in the debt entry, the C++ exceptions thrown through libjpeg's C frames, and the weakPsl.cppassertions) still apply. This commit does not touch those files, and I did not re-check them at this head.
Not verified
- Nothing was compiled or run; the test changes were read, not executed.
- Hosted checks are queued at this head.
- Console behaviour of the I/O callbacks is not measured.
To get this to a good state
Convert docs/dev/debt/libSceNKWebKitRequirements.md to LF and confirm git diff --check ba627802e2210187d417deb64c8f2fed8e33011f HEAD is empty. The diff check and the I/O regressions will be rerun on the new head.
✅ AMD GPU test: passedPR head
Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 717 tests)Skipped (57)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@MarcZX10 thanks, all addressed in 967de9b. Line endings (blocking). Non-blocking points:
Host harness: the zlib, jpeg, xml and psl tests pass. The changed tests compile with the real |
✅ AMD GPU test: passedPR head
Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 717 tests)Skipped (57)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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 — no blocking findings
The CRLF line endings that blocked the previous head are fixed at head 967de9b, and the three changes in this delta (a CMake cache change, a JPEG error-exit test and tighter PSL assertions) read correctly; hosted checks are still pending, so I am not approving.
What I ran
- Exact head
967de9b8893ca8dae0c6dc1da04728233073f943(one commit afterfbc2ded785, the head of my earlier request-changes review 5728122842), baseba627802e2210187d417deb64c8f2fed8e33011f. - Environment: Linux,
/usr/bin/python33.11.2, no build. - Precheck at the new head:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421 --output-dir /dev/shm/reviewer-run/precheck/14421b - Delta read with the compare API (
fbc2ded7...967de9b8): four files,cmake/LibXml2.cmake,Jpeg.cpp,Psl.cpp, and the debt doc. - Debt doc line endings checked with
gh apicontents at both heads: 0 CR bytes at967de9b8, 57 atfbc2ded7. - ABI audit by hand against the local repository, at both heads, to compare findings (the helper is absent from the head tree):
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base ba627802e2210187d417deb64c8f2fed8e33011f --head 967de9b8893ca8dae0c6dc1da04728233073f943 --json - Test-quality helper on
Jpeg.cppandPsl.cppfrom a head extract: each reports one literal-path registration heuristic; both are already registered in the library's test target.
Results
Precheck report, checks table, unedited (/dev/shm/reviewer-run/precheck/14421b/report.md):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check ba627802e2210187d417deb64c8f2fed8e33011f...967de9b8893ca8dae0c6dc1da04728233073f943 |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 967de9b8893ca8dae0c6dc1da04728233073f943 --title feat(libSceNKWebKitRequirements): implement the libjpeg, libxml2 and libpsl exports --body /dev/shm/reviewer-run/precheck/14421b/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 967de9b8893ca8dae0c6dc1da04728233073f943 --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14421 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14421b/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /dev/shm/reviewer-run/precheck/14421b/head-967de9b8893c-7d3f2d4f/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14421b/head-967de9b8893c-7d3f2d4f --json .gitmodules 3rdparty/libpsl 3rdparty/libxml2 cmake/LibPsl.cmake cmake/LibXml2.cmake core/libs/prx/libSceNKWebKitRequirements/CMakeLists.txt core/libs/prx/libSceNKWebKitRequirements/Export.cpp core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/Xml.cpp core/libs/prx/libSceNKWebKitRequirements/Zlib.cpp core/libs/prx/libSceNKWebKitRequirements/include/GuestJpeg.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestPsl.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestXml.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestZStream.hpp core/libs/prx/libSceNKWebKitRequirements/include/Identified.hpp core/libs/prx/libSceNKWebKitRequirements/tests/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cpp core/libs/tests/GuestWebKitZlib.cpp docs/dev/debt/libSceNKWebKitRequirements.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Manual results: the ABI audit at 967de9b8 reports passed_with_warnings with 28 findings and 195 symbols; at fbc2ded7 it reported 27. The one new finding is RecordingErrorExit, the test callback described below, flagged lexically as a definition with no header declaration.
Hosted at review time: conventions was cancelled and re-queued, and eligibility, library-portability and report were queued. No approval until these pass.
Negative control
Not run. The PR does not claim a negative control for the new JPEG test.
Findings
Blocking: none.
Non-blocking:
TestGuestErrorExitexpectsjpeg_read_headerto return 0 after the guesterror_exitreturns. libjpeg's contract is thaterror_exitdoes not return, so this asserts behaviour of the bundled libjpeg-turbo when a callback returns. That is worth a comment in the test, and the assertion should be confirmed on a build.RecordingErrorExitis markedAPS5_VABIinside the anonymous namespace; the ABI audit's new finding comes from that marker. Dropping the marker, or stating in the debt doc that it is test-local, would clear it.- The PSL assertions
wildCount > 0andexcCount > 0assume the bundled list contains wildcard and exception rules such as*.ckand!www.ck. The bundled list is in the libpsl submodule, which is not in this checkout, so I did not check it. cmake/LibXml2.cmakechangesset(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE)to a plainset. That is a function-scope variable, so it no longer writes the cache. Confirm on a configure that the bundled libxml2add_subdirectoryruns inside that function.
Sources
- Debt doc at head, LF: https://github.com/boykopovar/AnyPS5/blob/967de9b8893ca8dae0c6dc1da04728233073f943/docs/dev/debt/libSceNKWebKitRequirements.md
- JPEG test at head:
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/tests/Jpeg.cpp
Lines 230 to 261 in 967de9b
- CMake change at head:
Line 6 in 967de9b
Not verified
- No build, so the JPEG test and the PSL test were not compiled or run; their results are unknown on this head.
- libjpeg-turbo's behaviour when
error_exitreturns is not verified. - The bundled PSL rules were not checked against the libpsl submodule.
- Hosted: the CI jobs listed above were not complete at review time.
|
@MarcZX10 thanks. On the four non-blocking points, at 967de9b:
|
Progress report (d1ae628 → b75abdf)📈 System libraries: 26.66% (+0.19%, +70 implemented functions) ✅ 70 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, no blocking findings
The CRLF line endings and the libxml2 callback-slot problems that my earlier request-changes reviews raised are both resolved at head 8cabcc0; the only new change is a rename of five static default callbacks, and hosted checks are still pending, so this is a COMMENTED result, not an approval.
What I ran
- Exact head
8cabcc04effd5bff71a887b1a70e2753de68924b, baseba627802e2(the precheck base). The head moved from967de9b889(my last posted result, review 5728363081). The one new commit,8cabcc04ef, is a refactor incore/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp. - Environment: Linux,
/usr/bin/python33.11.2, no build. - Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421 --output-dir /dev/shm/reviewer-run/precheck/14421c - Delta read with the compare API (
967de9b8...8cabcc04): one file,Jpeg.cpp, 10 lines changed. - CRLF check: the debt file
docs/dev/debt/libSceNKWebKitRequirements.mdat this head has 0 CR bytes (fetched withgh api); atfbc2ded785it had 57. - libxml2 callbacks read at head:
core/libs/prx/libSceNKWebKitRequirements/Xml.cpplines 745-785.xmlRegisterInputCallbacks_nid_postfixandxmlRegisterOutputCallbacks_nid_postfixstore each guest callback set in its own slot (g_inputSlots[slot],g_outputSlots[slot]), bind it to a per-slot host trampoline (InputTrampolines[slot],OutputTrampolines[slot]), refuse when the slot count is reached, and clear the slot if libxml2 rejects the registration. Both slot counts are 15 (XmlInputSlotCount,XmlOutputSlotCount, lines 45-46). - ABI audit by hand (helper absent from the head tree):
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base ba627802e2 --head 8cabcc04effd5bff71a887b1a70e2753de68924b --json(exit 0;passed_with_warnings; 23public-declaration-not-foundfindings, all lexical; 195 symbols).
Results
Precheck report, checks table, unedited (/dev/shm/reviewer-run/precheck/14421c/report.md):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check ba627802e2210187d417deb64c8f2fed8e33011f...8cabcc04effd5bff71a887b1a70e2753de68924b |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 8cabcc04effd5bff71a887b1a70e2753de68924b --title feat(libSceNKWebKitRequirements): implement the libjpeg, libxml2 and libpsl exports --body /dev/shm/reviewer-run/precheck/14421c/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 8cabcc04effd5bff71a887b1a70e2753de68924b --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14421 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14421c/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /dev/shm/reviewer-run/precheck/14421c/head-8cabcc04effd-d0337903/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14421c/head-8cabcc04effd-d0337903 --json .gitmodules 3rdparty/libpsl 3rdparty/libxml2 cmake/LibPsl.cmake cmake/LibXml2.cmake core/libs/prx/libSceNKWebKitRequirements/CMakeLists.txt core/libs/prx/libSceNKWebKitRequirements/Export.cpp core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/Xml.cpp core/libs/prx/libSceNKWebKitRequirements/Zlib.cpp core/libs/prx/libSceNKWebKitRequirements/include/GuestJpeg.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestPsl.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestXml.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestZStream.hpp core/libs/prx/libSceNKWebKitRequirements/include/Identified.hpp core/libs/prx/libSceNKWebKitRequirements/tests/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cpp core/libs/tests/GuestWebKitZlib.cpp docs/dev/debt/libSceNKWebKitRequirements.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not applicable to this head: the change is a rename.
Findings
Blocking: none.
Non-blocking:
- The five default callbacks (
DefaultGuestErrorExit,DefaultGuestEmitMessage,DefaultGuestOutputMessage,DefaultGuestFormatMessage,DefaultGuestResetErrorMgr) arestatic, so the rename changes no linkage or behaviour; it only changes whattools/progress.pycounts. A rename to satisfy a counting tool is a workaround. The tool should skipstaticfunctions, and the rename can then be dropped.
Stale reviews: my earlier CHANGES_REQUESTED reviews 5728052613 (at ec1fa3c51e) and 5728122842 (at fbc2ded785) no longer describe this head. 5728052613 raised two things at ec1fa3c51e: libxml2 registrations sharing one set of guest callbacks, and CRLF debt-file line endings. The shared-callback defect is fixed by the per-registration slots at fbc2ded785 (read above); the CRLF item is fixed at 967de9b889 (0 CR bytes). 5728122842 raised the CRLF item at fbc2ded785, fixed at 967de9b889. Both should be dismissed or superseded by the author or coordinator.
Sources
- Registration code at head:
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Xml.cpp
Lines 745 to 785 in 8cabcc0
- Slot counts at head:
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Xml.cpp
Lines 45 to 46 in 8cabcc0
- The refactor at head: https://github.com/boykopovar/AnyPS5/blob/8cabcc04effd5bff71a887b1a70e2753de68924b/core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp
Not verified
- The pinned libxml2 callback limit (15) was verified in an earlier review of the same pin; the header was not re-fetched for this result.
- The C++ tests (
guest_nkwebkit_requirementsand the Jpeg, Psl and Xml tests) were not built or run here. - The PSL assertions against the bundled list were not checked (from my earlier review).
- Hosted: at this head
conventions(one run cancelled, one queued),eligibility,labels,library-portabilityandreportwere queued at review time.
✅ AMD GPU test: passedPR head
Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 717 tests)Skipped (57)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 8cabcc04effd5bff71a887b1a70e2753de68924b.
[P1] Unwrap guest FILE streams before passing them to libpsl — Psl.cpp:76–77. psl_load_fp_nid_postfix forwards its argument directly to host psl_load_fp. Guest stdio uses FileStream*, whose host FILE* is stored beyond the guest FILE prefix; the two pointers are not interchangeable. Reproduced on Windows with the pinned libpsl: loading the same two-rule suffix list through a host FILE passes, while passing a real FileStream wrapper segfaults. A separate control that unwraps the stream with GetStreamHandle passes. Please expose the guest stream type, unwrap it for libpsl, and make the regression use a guest stream rather than std::fopen's host pointer.
Validation: built the submitted shared library against the pinned JPEG/XML/PSL dependencies with MSYS2 GCC 16.2 Release; all four supplied suites pass. The supplied PSL test also passes standalone, but its FILE test uses a host FILE and misses this defect. Conventions and whitespace pass. The diagnostic control changes only the review harness; this PR's files are unchanged. Depends on #14374.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
Three blocking defects at this head: a guest FILE* reaches a host FILE API, a guest document pointer dangles after its parser context is freed, and the default JPEG error callback returns after a fatal error.
What I ran
- Head
8cabcc04effd5bff71a887b1a70e2753de68924b; PR baseba627802e2; merge baseba8dcc6ae8; 21 changed files, +4012/-351 (PR files API). - Read: all 21 changed files, split across five read-only subagents (Xml.cpp; Jpeg.cpp, GuestJpeg.hpp, tests/Jpeg.cpp; GuestXml.hpp, tests/Xml.cpp, Identified.hpp, GuestPsl.hpp; Zlib.cpp, GuestWebKitZlib.cpp, GuestZStream.hpp, Psl.cpp, tests/Psl.cpp; Export.cpp removals, CMake files, debt doc, submodule gitlinks). I re-read the three blocking sites and the CONTRIBUTING rule myself. The non-blocking items are the subagents' findings; I re-checked only the lines cited under Findings.
- Precheck report at this exact head (head and base unchanged at completion). Two precheck rows (ABI audit, test quality) failed; I reran both helpers by hand on an archive of the head. Details below.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | |
| Mergeability and dependencies | passed | live check |
| Export ABI audit (by hand) | passed | 195 symbols, 0 alias mismatches; 23 warnings are test-file callbacks |
| Test quality (by hand) | passed | 4 flags are false positives: the tests are registered in CMake |
| Guest FILE handling | failed | host FILE API on a guest stream (finding 1) |
| Document lifetime | failed | pointer outlives its context (finding 2) |
| Error callbacks | failed | returns after a fatal error (finding 3) |
| Build and runtime tests | not run | no build |
| Negative controls | not run | needs a build |
| Hosted CI | not run | at review time Windows was in progress and macOS relinker jobs were queued; completed runs show no failure |
Commands and unedited precheck output
Commands:
- Precheck: as listed in the precheck table below (report at /dev/shm/reviewer-run/precheck/14421c/report.md).
- ABI audit, by hand, from the main checkout:
python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 8cabcc04effd5bff71a887b1a70e2753de68924b --json. Result: status passed_with_warnings; 195 symbols; 0 alias mismatches; 23 findings, all of kind public-declaration-not-found. The 23 are defined only in tests/Jpeg.cpp (GuestFillInputBuffer, GuestInitSource, GuestResyncToRestart, GuestSkipInputData, GuestTermSource, RecordingErrorExit) and tests/Xml.cpp (IoClose, IoMatch, IoOpen, IoRead, OutClose, OutMatch, OutOpen, OutWrite, SaxCdataBlock, SaxCharacters, SaxComment, SaxEndDoc, SaxEndElementNs, SaxError, SaxSerror, SaxStartDoc, SaxStartElementNs). - Test quality, by hand, on an archive of the head:
python3 tools/pr_test_quality.py --root <archive> --json tests/Jpeg.cpp tests/Psl.cpp tests/Xml.cpp core/libs/tests/GuestWebKitZlib.cpp. Result: 4 possible-unregistered-cpp-test flags. Registration exists: CMakeLists.txt line 21 (guest_webkit_zlib_tests) and line 31 (foreach over Jpeg, Xml, Psl). - Hosted checks:
gh api repos/boykopovar/AnyPS5/commits/8cabcc04effd5bff71a887b1a70e2753de68924b/check-runs.
Precheck table (unedited):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check ba627802e2210187d417deb64c8f2fed8e33011f...8cabcc04effd5bff71a887b1a70e2753de68924b |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 8cabcc04effd5bff71a887b1a70e2753de68924b --title feat(libSceNKWebKitRequirements): implement the libjpeg, libxml2 and libpsl exports --body /dev/shm/reviewer-run/precheck/14421c/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head 8cabcc04effd5bff71a887b1a70e2753de68924b --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14421 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14421c/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /dev/shm/reviewer-run/precheck/14421c/head-8cabcc04effd-d0337903/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14421c/head-8cabcc04effd-d0337903 --json .gitmodules 3rdparty/libpsl 3rdparty/libxml2 cmake/LibPsl.cmake cmake/LibXml2.cmake core/libs/prx/libSceNKWebKitRequirements/CMakeLists.txt core/libs/prx/libSceNKWebKitRequirements/Export.cpp core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/Xml.cpp core/libs/prx/libSceNKWebKitRequirements/Zlib.cpp core/libs/prx/libSceNKWebKitRequirements/include/GuestJpeg.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestPsl.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestXml.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestZStream.hpp core/libs/prx/libSceNKWebKitRequirements/include/Identified.hpp core/libs/prx/libSceNKWebKitRequirements/tests/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cpp core/libs/tests/GuestWebKitZlib.cpp docs/dev/debt/libSceNKWebKitRequirements.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run. The new tests are C++ and need a build.
Findings
Blocking
-
A guest FILE is passed to a host FILE API.*
Identified.hppline 52 declarespsl_load_fp_nid_postfix(FILE* fp)with the hostFILE, andPsl.cpplines 76-78 pass it unchanged to libpsl'spsl_load_fp. In this libraryfopenreturns aFileStream*, and the hoststd::FILE*sits behind the guest prefix (FileStream.hpplines 64-68), so libpsl would run stdio on aFileStreamobject.tests/Psl.cpplines 103-105 use the hoststd::fopen, so the test cannot catch this. Smallest fix: take the guest stream type and pass its native handle; add a test that goes through the libcfopenpath. -
A document outlives its parser context.
Xml.cppline 190 setsg->myDoc = &b->guest_doc, andguest_docis a member of the binding (line 37).xmlFreeParserCtxt_nid_postfix(lines 694-703) moves the binding out of the map (line 696), freeshost_ctxt(line 701), and destroys the binding, includingguest_doc, when it returns. A guest that takesctxt->myDoc, frees the context and keeps using the document reads freed memory. Smallest fix: give the guest document storage its own owner, released with the document, not with the binding; add a test that frees the context before the document. -
The default JPEG error callback returns after a fatal error.
Jpeg.cpplines 152-156 (DefaultGuestErrorExit_nid_no_patch) only calloutput_messageand return.HandleException(lines 314-329) callserror_exit(line 326) and returns; the wrapperjpeg_calc_output_dimensions(lines 470-480) catchesJpegException(line 477), callsHandleException(line 478) and returns normally, so a fatal decode error becomes a normal return. CONTRIBUTING.md line 7 requires each function to do what it is supposed to or throw. Smallest fix: make the defaulterror_exitthrow.tests/Jpeg.cppreplaceserror_exit(line 242), so the default path is not tested; add a test for it.
Non-blocking (subagent findings; line numbers as reported except where cited above)
- JPEG: the
jpeg_CreateDecompresscatch does not copymsg_code(374-378).s_std_initedis set without synchronisation (338-341). Source-manager callbacks return success when the guest callback is NULL (93-122). The exception object leaks if a guesterror_exitlongjmps out of the catch (423). - XML: the Host callbacks call guest code without
SyncHostToGuest(781-799). A reentrantSyncGuestToHostcan restore a stalespaceTab(663, 730, 739, 819).h->charset = h->charsetshould readg->charset(226). Six SAX slots are never forwarded andhost_saxstarts zeroed (502-528).guest_ctxt.saxis never set (608-625).FindBindingreleases the mutex before returning a raw pointer (68-76).InitHostSaxsetsinitializedunconditionally (504). - Zlib (stacked on #14374): the licence blocks and attribution comment (Zlib.cpp 1-35) and the adapted
uncompress(231-269) are maintainer questions under the third-party code rules; CONVENTIONS allows only debt comments, but the same kind of header exists in libc sources. The branch at 151 leaks on a path the pinned zlib does not reach; a re-initialisation leaks the old stream (148). The totals are checked only on zero-initialised streams (GuestWebKitZlib.cpp 102-108 and 122-128). - PSL: the
outdatedcheck accepts 0 or 1 and so proves nothing (tests/Psl.cpp 40-41). The builtin list name returned at Psl.cpp 20-22 comes from the build configuration; whether it embeds a build-host path was not confirmed. IDN support is disabled, so the version string reports "no IDNA support", and the debt doc does not record it. - Build:
cmake/LibPsl.cmakerequires Python 3 at every configure.cmake/LibXml2.cmakekeeps the libxml2 install rules and turns off XInclude, XPath, Reader, Writer, zlib and gzip input; the debt doc lists none of this. - Tests:
TestErrorHandlingnever asserts the error callback (tests/Xml.cpp 174); thexmlSwitchEncodingresult is discarded (200-201); the external-entity text is not checked (344-358); the progressive JPEG test does no pixel comparison and has unbounded loops (182-226).
Sources
- Psl: and
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Psl.cpp
Lines 76 to 78 in 8cabcc0
- Stream type:
AnyPS5/core/libs/prx/libc/include/FileStream.hpp
Lines 64 to 68 in 8cabcc0
- Xml document: and
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Xml.cpp
Lines 188 to 190 in 8cabcc0
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Xml.cpp
Lines 694 to 703 in 8cabcc0
- JPEG callback and handler: , #L314-L329 and #L470-L480
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp
Lines 152 to 156 in 8cabcc0
- Rule:
Line 7 in 8cabcc0
Not verified
- Guest call patterns (not in the repository): whether a guest takes
myDocand frees the context, and whether a guest'serror_exitlongjmps. - libxml2 2.9.14 ownership of
myDocon context free was not checked against the pinned source. The submodule is not checked out; the pin itself was checked. - The libpsl and libxml2 sources are not checked out locally; the NID values were not recomputed here (the names are unchanged between base and head).
- A subagent report said warning counts and trace level are not propagated to the guest; the code at Jpeg.cpp lines 323-324 does copy them, so that claim is not carried forward.
- Build, tests and negative controls were not run. Hosted CI was still running at review time.
To get this to a good state
- Fix the three blocking items, each with a test:
psl_load_fpthrough the libc stream path; a document read after its context is freed; the default JPEGerror_exitthrows. - Rerun the precheck and the Jpeg, Xml and Psl test executables on the new head.
|
@MarcZX10 @THEMANnt thanks, the three blocking items are fixed in a320213, each with a regression. 1. Guest FILE* to libpsl. 2. Document outliving its context. The binding no longer holds the guest document.
Against the previous 3. Default Verification:
The non-blocking items are not part of this commit. |
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 717 tests)Skipped (57)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Independent Windows integrated-build result for a320213: build succeeds, but the JPEG test aborts. Environment:
Built successfully: CTest results:
The JPEG log reports: This is the internal exception thrown by HostErrorExit, not the std::runtime_error expected from the default guest error handler. The source has catch handlers for JpegException, so exception propagation in the integrated Windows build seems worth investigating; the exact cause is not established. Reproduction after configuring the project: cmake --build build --parallel 8 --target guest_webkit_zlib_tests guest_nk_req_jpeg_tests guest_nk_req_xml_tests guest_nk_req_psl_tests No source changes were applied. This validates only the focused build/tests, not title execution. AI-assisted: yes (ChatGPT), for commands, log analysis and this report. Tests executed locally on my Windows machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The three blockers from my earlier review are fixed at this head, but the Windows job still aborts in guest_nk_req_jpeg (a libjpeg error path throws through C frames), as it did at 8cabcc0. This review at a320213 supersedes 5728873006 (8cabcc0) for the current head.
What I ran
- Head
a32021388e2d09040667e76c72f3fa56d8d38672; baseba627802e2; merge baseba8dcc6ae8; 21 changed files, +4101/-351, 9 commits (PR files API). - Change since my last review head
8cabcc04ef: one commit (a32021388e), 8 files, +101/-12. I read the whole delta with context myself and checked each fix against the libcFileStreamAPI and the libjpeg error path. The other 13 PR files are byte-identical to8cabcc04ef(verified withgit diff --statbetween the two heads); the five read-only subagent reports from that review still cover them. I did not re-read those 13 files. - Precheck at this exact head (head and base unchanged at completion). Two precheck rows (ABI audit, test quality) failed because their helpers are not in the head tree; I reran both by hand on an archive of the head. Details below.
- Hosted Windows job logs at this head and at
8cabcc04ef, to compare the abort.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | |
| Mergeability and dependencies | passed | stacked on #14374 (still open) |
| Export ABI audit (by hand) | passed | 195 symbols, 0 alias mismatches; 23 warnings are test-file callbacks |
| Test quality (by hand) | passed | 4 flags are false positives; the tests are registered in CMake |
| Blocker 1: guest FILE* in libpsl | passed | guest stream is unwrapped; null-stream and end-of-file tests added |
| Blocker 2: document past its context | passed | documents are owned until xmlFreeDoc; test added |
| Blocker 3: default libjpeg error_exit | passed | now throws; test added |
| Hosted Windows job | failed | guest_nk_req_jpeg terminates on a libjpeg error; same at 8cabcc0 |
| Other hosted jobs | passed | conventions, eligibility, labels, library portability, relinker (ubuntu, windows, asan), report; macOS relinker queued |
| Build and runtime tests | not run | the Windows log is the only runtime evidence |
| Negative controls | not run | needs a build |
Commands, logs and unedited precheck output
Commands:
- Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421 --output-dir <scratch>/precheck/14421d(report.md in that directory). - ABI audit, by hand, from the main checkout:
python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head a32021388e2d09040667e76c72f3fa56d8d38672 --json. Result: passed_with_warnings; 195 symbols; 0 alias mismatches; 23 public-declaration-not-found warnings, all in test files. - Test quality, by hand, on an archive of the head, on tests/Jpeg.cpp, tests/Psl.cpp, tests/Xml.cpp and core/libs/tests/GuestWebKitZlib.cpp. Result: 4 possible-unregistered-cpp-test flags; the tests are registered in CMakeLists.txt.
- Windows log: run 41333345534, job 124934749616 (head a320213). Lines:
guest_nk_req_jpeg ... Exit code 0xc0000409,unhandled exception 'JpegException' thrown,terminate called after throwing an instance of '(anonymous namespace)::JpegException'. The same test, with the same messages, failed at 8cabcc0: run 41316484095, job 124885806921.
Precheck table (unedited):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check ba627802e2210187d417deb64c8f2fed8e33011f...a32021388e2d09040667e76c72f3fa56d8d38672 |
| conventions_and_pr_template | passed | /marc/Conlyse-VIEJO/.venv/bin/python3 tools/check_conventions.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head a32021388e2d09040667e76c72f3fa56d8d38672 --title feat(libSceNKWebKitRequirements): implement the libjpeg, libxml2 and libpsl exports --body /dev/shm/reviewer-run/precheck/14421d/pr-body.md |
| changed_export_abi_audit | failed | /marc/Conlyse-VIEJO/.venv/bin/python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head a32021388e2d09040667e76c72f3fa56d8d38672 --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /marc/Conlyse-VIEJO/.venv/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14421 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14421d/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /marc/Conlyse-VIEJO/.venv/bin/python3 /dev/shm/reviewer-run/precheck/14421d/head-a32021388e2d-f482c71c/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14421d/head-a32021388e2d-f482c71c --json .gitmodules 3rdparty/libpsl 3rdparty/libxml2 cmake/LibPsl.cmake cmake/LibXml2.cmake core/libs/prx/libSceNKWebKitRequirements/CMakeLists.txt core/libs/prx/libSceNKWebKitRequirements/Export.cpp core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/Xml.cpp core/libs/prx/libSceNKWebKitRequirements/Zlib.cpp core/libs/prx/libSceNKWebKitRequirements/include/GuestJpeg.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestPsl.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestXml.hpp core/libs/prx/libSceNKWebKitRequirements/include/GuestZStream.hpp core/libs/prx/libSceNKWebKitRequirements/include/Identified.hpp core/libs/prx/libSceNKWebKitRequirements/tests/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cpp core/libs/tests/GuestWebKitZlib.cpp docs/dev/debt/libSceNKWebKitRequirements.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run. The new tests are C++ and need a build.
Findings
Blocking
- The Windows job aborts in guest_nk_req_jpeg. The log shows a
JpegExceptionleaving the test and callingterminate. The throw comes from the host error callback (Jpeg.cppline 41,throw JpegException{cinfo}), which runs inside libjpeg's C code; the C++catchin the wrappers is above those frames. On Windows, C frames built without unwind information cannot be unwound through, which is consistent with the abort. The same abort is in the job at 8cabcc0, so this predates this commit; the commit does not change this path. I did not check the libjpeg-turbo build flags for Windows. Direction, not verified: keep libjpeg's error path unwind-safe on Windows (build flags for the C sources), or leave the error through libjpeg's own setjmp path instead of throwing through it. Hosted checks must pass before approval.
Blockers 1 to 3 from the earlier review: fixed at this head
- Psl:
psl_load_fp_nid_postfixnow takes the guestFileStream*(Psl.cppline 76,Identified.hppline 52), returns null for a null stream, and passes the native handle fromGetNativeStream(FileStream.hppline 401).SyncStatus(line 252) publishes end-of-file and error state, as other libc functions do when they read the native handle. Tests attests/Psl.cpplines 105-110 go through the guestfopenpath and checkfeof,fcloseand a null stream. - Xml: the guest document is now owned by
g_guestDocs, keyed by the host document (Xml.cpplines 43, 140-143, 196-198), and released inxmlFreeDoc(lines 681-696), not with the parser binding.tests/Xml.cpplines 417-443 free the context and then use the document. - Jpeg: the default
error_exitnow throwsstd::runtime_errorwith the formatted message (Jpeg.cpplines 153-159).tests/Jpeg.cpplines 265-291 check the throw andJERR_NO_SOI. The debt file (line 12) records this.
Non-blocking
- Jpeg,
jpeg_CreateDecompress(lines 377-384): the catch calls the guesterror_exitwithout copyingmsg_codefrom the host error, unlike the other wrappers (lines 323-328). The defaulterror_exitformats from the guestmsg_code, so the message on this path can be stale. - Jpeg, lines 342-345:
s_std_initedis set without synchronisation. - Jpeg: an exception object leaks if a guest
error_exitlongjmps out of a catch block (reported; not re-run). - Xml, line 234:
h->charset = h->charset;should readg->charset. - Xml, lines 510-512:
InitHostSaxsetsinitializedunconditionally. Host SAX slots are forwarded only when the guest slot is set (lines 513-525). - Xml, line 232:
SyncGuestToHostwritesh->spaceTabfrom the guest; a reentrant sync can restore a stale value (reported; not re-run). - Xml, lines 68-76:
FindBindingreleases the mutex before its raw pointer is used. - Xml: no assignment to
guest_ctxt.saxexists in the file, so the guest SAX table is not set on the guest context (grep). - Xml tests:
TestErrorHandling(tests/Xml.cppline 171) never asserts the error callback; thexmlSwitchEncodingresult at line 200 is discarded (line 201). - Psl test, lines 42-43:
outdated == 0 || outdated == 1proves nothing. - Jpeg tests, lines 210 and 220: the progressive loops have no bound (reported).
- Psl, lines 20-22: the builtin list name comes from the build configuration; whether it embeds a build-host path was not confirmed.
- Unchanged files, carried from the earlier review: Zlib.cpp licence and attribution headers and the adapted
uncompress(maintainer questions); the leak at Zlib.cpp line 151 and the re-initialisation leak at line 148; the zero-initialised checks at GuestWebKitZlib.cpp lines 102-108 and 122-128; CMake requires Python 3 on every configure; libxml2 install rules and the disabled features are not all in the debt file.
Sources
- Blocker 1 (Psl): and
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Psl.cpp
Lines 76 to 82 in a320213
and #L401-L405AnyPS5/core/libs/prx/libc/include/FileStream.hpp
Lines 252 to 255 in a320213
- Blocker 2 (Xml): and #L140-L143 and #L196-L198 and #L681-L696
- Blocker 3 (Jpeg): and #L153-L159 and #L377-L384
- Windows failure: https://github.com/boykopovar/AnyPS5/actions/runs/41333345534/job/124934749616 (same test at the previous head: https://github.com/boykopovar/AnyPS5/actions/runs/41316484095/job/124885806921)
Not verified
- No build or test run. The Windows job log is the only runtime evidence; the linux job is skipped for this PR, so the Linux runs of these tests were not observed.
- How a C++ exception from the guest-called default
error_exitreaches the guest boundary was not traced. Guest code can also call that function through the pointer. - The libjpeg-turbo build flags on Windows were not checked.
- The libxml2 ownership of
myDocon context free was not checked against the pinned source. - The 13 unchanged PR files were not re-read at this head (see What I ran).
To get this to a good state
- Make the libjpeg error path safe on Windows (or leave it without a C++ throw through libjpeg frames), so guest_nk_req_jpeg passes in the Windows job.
- Rerun the precheck and the Jpeg, Xml and Psl test executables on the new head.
- Address the non-blocking items as the author sees fit.
|
@MarcZX10 thanks, fixed in 1946ca0. Windows abort in guest_nk_req_jpeg. On MinGW the libraries unwind with DWARF tables (
Non-blocking item, also fixed: The debt entry now describes the jump instead of exceptions. Host harness (WinLibs GCC): the jpeg, xml and psl tests pass, including |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
Four blocking defects at head 1946ca063c: the Xml use-after-free, two Xml pointer forwards, and the JPEG stale-position bug are verified by reading the exact lines, and Zlib.cpp carries block comments that the repository's rule forbids.
What I ran
- Exact head
1946ca063c119301a8a1212063c0e81bd6ec7b2f, PR baseba627802e2210187d417deb64c8f2fed8e33011f. - Read-only:
git -C /marc/AnyPS5 show 1946ca063c119301a8a1212063c0e81bd6ec7b2f:<path>for the changed files, split across read-only file-range reads. I re-read the lines cited below myself. - Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421 --output-dir <scratch>. - Not built, not run.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions | passed | comment findings dropped because the PR touches a debt file |
| ABI audit | failed | helper absent from the head tree |
| Test quality | failed | helper absent from the head tree |
| Build and ctest | not run | needs a build |
| Windows hosted job | not verified | see Not verified |
Precheck report (unedited header and failing rows)
# PR precheck report — checks failed
changed_export_abi_audit | failed | pr_abi_audit.py (helper absent from the head tree)
test_quality_heuristics | failed | pr_test_quality.py (helper absent from the head tree)
Findings
Blocking
core/libs/prx/libSceNKWebKitRequirements/Xml.cpp698-699:xmlFreeDocreleases the host document, but nothing clears the host parser context'smyDoc. The next sync (195-198) reads the freed document. Fix: clearmyDocon every binding that points at the freed document, under the same lock.Xml.cpp812:xmlSAX2StartDocument(b ? b->host_ctxt : ctx)passes the guest pointer to libxml2 when no binding is found. The same pattern appears at 657, 797, 802 and 807 (reported). Fix: return without calling libxml2 whenbis null.core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp104-137: the guest source callbacks see the guest's input position from before the libjpeg call. libjpeg advances the host copy, and the write-back at 137 overwrites it with the stale value. Syncs happen only at entry and exit. Fix: copy the host state into the guest source before each guest callback. Add a test with an unsaved APPn marker that checks pixels.core/libs/prx/libSceNKWebKitRequirements/Zlib.cpp1-4 (the comment block runs to line 35): block comments with the zlib attribution and copyright. CONVENTIONS.md line 13 allows comments only for debt. Fix: remove them and put the attribution in the PR description, or ask maintainers for an exception.
Non-blocking
- The Windows abort in
guest_nk_req_jpegappears fixed:Jpeg.cpp36-54 replaces the C++ throw withlongjmp, and the only throw left runs after libjpeg has returned. Not confirmed by a Windows run. tests/Psl.cpp28 expectspsl_check_version_number_nid_postfix(0x001000) == 0. A minimum-version check should not return 0 for a library that is newer. Reported, not run. It needs a run before merge.Jpeg.cpp380 does not checkstructsize(reported).- The debt file overstates what the wrappers guarantee (reported, not confirmed line by line).
Sources
- Xml frees: and #L698-L699
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Xml.cpp
Lines 195 to 198 in 1946ca0
- Xml forward:
- JPEG callbacks:
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Jpeg.cpp
Lines 104 to 137 in 1946ca0
- Zlib comments:
AnyPS5/core/libs/prx/libSceNKWebKitRequirements/Zlib.cpp
Lines 1 to 4 in 1946ca0
Not verified
- Build, ctest and every new test: not run.
- The Windows hosted job at this head: not confirmed.
- libxml2 and libpsl sources are submodules that are not in this checkout, so their behaviour is unverified.
- libjpeg behaviour for the stale-position case: reasoned from the code, not run.
To get this to a good state
Fix blocking 1-4, rebuild, and run guest_nk_req_xml, guest_nk_req_jpeg and guest_nk_req_psl on the new head. Confirm the Windows job passes.
✅ AMD GPU test: passedPR head
Unstable on main (6, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 717 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Correction — the Psl version-check item is wrongThe item on |
|
@MarcZX10 thanks. Blocking items 1 to 3 are fixed in d2316b7, and the 1.
Against the previous 2. Guest pointers forwarded to libxml2.
3. Stale input position in guest source callbacks. The new test decodes a baseline JPEG whose APP1 marker is not saved, so libjpeg skips it through the guest A plain unsaved marker was not enough: with the stale position, libjpeg resynchronised on the next real marker and only warned about extraneous bytes. So the skipped payload carries an 4. The comment block in
@boykopovar, could you confirm that a third-party license notice on adapted code is accepted as an exception to the no-comments rule, as on Non-blocking.
Run. Host harness (WinLibs MinGW, x86-64), with negative controls against the previous
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
At head bb9960ffa0d11520159f16adb43c283c67e4f8e1, the new CMake dependency breaks the documented Python-optional configure path, and guest updates to the XML parser context’s charset are discarded.
What I ran
Reviewed all 21 changed file entries at head bb9960ffa0d11520159f16adb43c283c67e4f8e1 against PR base ba627802e2210187d417deb64c8f2fed8e33011f. The dependency-aware merge preview against current main 88e804b0a7df1f1ac373a9e2f72fb378d925fdd4 was clean; #14374 is included in this PR’s stack. Environment: Debian Linux, Python 3.11.2, GCC 12.2.0; no build or compilation was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421 --output-dir /tmp/review-14421-precheck
python3 tools/pr_abi_audit.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head bb9960ffa0d11520159f16adb43c283c67e4f8e1 --json
python3 tools/pr_test_quality.py --root /tmp/review-14421-precheck/head-bb9960ffa0d1-9847ce5d --json core/libs/prx/libSceNKWebKitRequirements/tests/Jpeg.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Psl.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cpp core/libs/tests/GuestWebKitZlib.cpp
git diff --check ba627802e2210187d417deb64c8f2fed8e33011f..bb9960ffa0d11520159f16adb43c283c67e4f8e1
python3 tools/pr_stack_check.py 14421 --repo boykopovar/AnyPS5 --output-dir /tmp/review-14421-stack-final
gh pr checks 14421 --repo boykopovar/AnyPS5Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | One debt-length warning |
| ABI audit | passed | 23 declaration warnings need manual interpretation |
| Test registration | passed | Heuristic warnings are registered through a CMake loop |
| Merge preview | passed | Clean against current main; #14374 included |
| Windows and relinker CI | passed | Current-head jobs passed |
| Linux CI | not run | Hosted job was skipped |
| Build and CTest | not run | No build in this initial review |
Precheck results (unedited)
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check ba627802e2210187d417deb64c8f2fed8e33011f...bb9960ffa0d11520159f16adb43c283c67e4f8e1 |
| conventions_and_pr_template | passed | tools/check_conventions.py --base ba627802e2210187d417deb64c8f2fed8e33011f --head bb9960ffa0d11520159f16adb43c283c67e4f8e1 |
| changed_export_abi_audit | failed | The isolated PR snapshot does not contain tools/pr_abi_audit.py; the workspace copy was run separately. |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools module has a sibling suite. |
| python_test_registration | not_applicable | No changed Python tests. |
| live_dependency_and_mergeability_check | passed | tools/pr_stack_check.py 14421 --repo boykopovar/AnyPS5 |
| build_dependent_cpp_tests | not_applicable | Exact-head binaries require a build; the precheck does not build. |
| test_quality_heuristics | failed | The isolated PR snapshot does not contain tools/pr_test_quality.py; the workspace copy was run separately. |
| negative_controls | not_applicable | No changed Python test module supports a no-build control. |
Negative control
Not run locally. The author reports regressions against prior code for the XML lifetime and JPEG callback fixes; I did not reproduce them. The current Windows CI is hosted evidence, not a local run.
Findings
Blocking
- Python is no longer optional at configure time.
core/libs/CMakeLists.txtadds every PRX directory, which includes this library. ItsCMakeLists.txtunconditionally includescmake/LibPsl.cmake, wherefind_package(Python3 COMPONENTS Interpreter REQUIRED)runs even though the library target isEXCLUDE_FROM_ALL. This makes a default CMake configure fail on systems without Python, contrary to the repository’s documented optional-Python path. Scope the Python requirement to building the PSL target or avoid requiring Python during configure. - The XML charset field is not synchronized from guest to host.
SyncGuestToHostassignsh->charset = h->charsetatXml.cpp:241, so a guest update to the exposed parser-context field is discarded before the next libxml2 call. Copy the guest field, or explicitly document and enforce that this exposed field is read-only. - The Zlib source notice still conflicts with the repository comment rule.
Zlib.cpp:1-35adds source comments, whileAGENTS.mdanddocs/dev/CONVENTIONS.mdpermit code comments only for technical debt and the specified closing markers. The PR author identifies the notice as required for the adapted zlib source; I am not asking that it be removed. Resolve this through a maintainer-approved exception or other license-compliant repository policy before merge.
Non-blocking
- The manual ABI audit reported 23
public-declaration-not-foundwarnings. These are lexical findings for internal callback/helper definitions; the audit does not establish an ABI defect. - The test-quality heuristic flagged four C++ tests as possibly unregistered.
CMakeLists.txtregisters them throughforeach(topic IN ITEMS Jpeg Xml Psl), so these are heuristic false positives. SyncGuestToHostalso leavesdictandsax2untouched; I did not establish whether guest writes to those opaque pointers are supported by this wrapper.
Sources
- Configure path and optional Python policy: core/libs/CMakeLists.txt, library CMakeLists.txt, LibPsl.cmake, CONTRIBUTING.md.
- XML field sync: Xml.cpp, GuestXml.hpp.
- Comment rule and retained license text: AGENTS.md, CONVENTIONS.md, Zlib.cpp.
- Fixes described by the author: PR discussion.
Not verified
- Linux build and tests were skipped by hosted CI; no local build, CTest, GPU, or console run was performed.
- The audit-only public-header command did not finish before I stopped it; it has no result in this review.
- The libjpeg-turbo, libxml2, and libpsl submodule contents were not checked out locally. The reviewed PR pins their gitlink revisions.
- Whether guest software mutates
charset,dict, orsax2in practice was not confirmed from a runtime caller.
To get this to a good state
Make the PSL generator optional at configure time, correct or formally restrict the XML charset sync, and resolve the Zlib notice policy with maintainers while preserving required license terms. Then run the applicable build and tests on the new head; I will rerun the same no-build checks and re-review the fixes.
|
@MarcZX10 thanks. 203d118 fixes items 1 and 2. Item 3 still needs a maintainer decision. 1. Python at configure time. 2. 3. zlib notice. Still waiting on a maintainer exception, which I asked for earlier in this thread. I'll do whatever the maintainers decide. I also shortened the zlib debt entry below the 1000-character limit, so |
Implement deflateInit2_, deflate, deflateReset, deflateEnd, deflateBound, inflateInit2_, inflate, inflateReset, inflateEnd and uncompress on the vendored zlib, translating the guest LP64 z_stream and calling the guest allocators through the guest ABI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…p gzip throw tests Keep the zlib 1.3.1 uncompr.c and zlib.h notices at the top of Zlib.cpp and mark how uncompress_nid_postfix alters uncompress2. Remove the test assertions that pinned the unimplemented gzip paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…idth On a host with a 32-bit uLong, a checksum zlib resets to the truncated value of the guest's 64-bit field was not written back, which left stale upper bits. Compare in guest width and cover initialization over a stale checksum. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…libpsl exports The 60 exports are adapters over libjpeg-turbo, libxml2 2.9.14 and libpsl 0.21.5, built from source (libxml2 and libpsl as new submodules). Guest-visible structures use LP64 mirrors, guest callbacks go through host trampolines, and invalid libjpeg objects throw instead of returning.
… guest callbacks Every successful xmlRegisterInputCallbacks or xmlRegisterOutputCallbacks now binds its own host trampolines to its own guest callbacks, so a later or failed registration no longer redirects the entries or the open contexts of earlier ones. A null guest callback is registered as null, as libxml2 stores it. A push parser created without a SAX handler now passes the caller's user data to libxml2 instead of the adapter's binding.
…ten the jpeg and psl tests The debt file goes back to LF line endings and says that I/O callback slots are kept for the process lifetime. LibXml2.cmake no longer forces BUILD_SHARED_LIBS into the cache. The jpeg test drives a guest error_exit through JERR_NO_SOI, and the psl test checks exact builtin metadata.
…callbacks as internal The five default guest error-manager callbacks take the _nid_no_patch suffix, so tools/progress.py no longer counts them as exports.
…p documents past their parser context and make the default libjpeg error_exit throw psl_load_fp takes the guest FileStream and passes its native stream to libpsl, then publishes the stream status. The guest document of a parser context is owned by the document and released by xmlFreeDoc, not by the context binding, so it stays valid after xmlFreeParserCtxt. The error_exit installed by jpeg_std_error throws std::runtime_error with the formatted message where libjpeg would exit, so a fatal error no longer becomes a normal return. Each fix has a regression: psl_load_fp through the libc fopen path, a document used after its context and a second context are freed, and the default error_exit on a stream without SOI.
…instead of a C++ exception On Windows the libraries unwind with DWARF tables that the libjpeg-turbo build does not emit, so a JpegException thrown from the host error_exit could not cross libjpeg's C frames and terminated guest_nk_req_jpeg. The host error_exit now jumps back to the export with __builtin_longjmp to a buffer each call sets up and restores, so nested exports keep their own target. jpeg_CreateDecompress also copies the error code to the guest before calling its error_exit, and jpeg_has_multiple_scans and jpeg_input_complete report libjpeg errors the same way.
… reject unknown contexts and keep the guest input position current xmlFreeDoc left the host parser context's myDoc pointing at the freed document, and the next sync of that context read it. It now clears myDoc in every context that still points at the document. xmlCtxtGetLastError and the xmlSAX2 exports passed a guest pointer to libxml2 when it was not a context this library created. A null context now returns as libxml2 does, and an unknown one throws std::invalid_argument. The guest source callbacks saw the input position from before the libjpeg call, so a guest skip_input_data advanced from a stale position. The host position is now copied into the guest source manager before each callback.
…ng jpeg_CreateDecompress structsize jpeg_CreateDecompress passed the host structure size to libjpeg and ignored the guest's, so a caller built against a different jpeg_decompress_struct was accepted. A structsize other than the guest mirror's size now raises JERR_BAD_STRUCT_SIZE through the guest error_exit with the expected and passed sizes, as libjpeg does, and leaves the object unusable.
…arser and need Python only to generate the suffix table The guest-to-host context sync assigned the host charset to itself, so a charset the guest set was lost on the next call. Configuring no longer requires a Python interpreter; building the libpsl suffix table without one fails with a message naming Python3_EXECUTABLE.
203d118 to
31dabab
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The entity-returning XML exports reuse one mutable mirror and clear the owning-document link, so valid entity lookups can return incorrect guest data.
What I ran
Reviewed all 21 changed files at head 31dabab2080fe1d6e94db59aaa477f1cf4479b5c against PR base b28bb772177768f99874ca38f6252cd0139acf2a. The merge preview against current main (7feb37ee3e30e10139b483fb4df689d5b5b69ab8) was clean. Environment: Debian Linux 6.1, Python 3.11.2, GCC 12.2.0. No local build or compilation was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421 --output-dir /tmp/anyps5-pr-precheck/14421-20261103
git diff --check b28bb772177768f99874ca38f6252cd0139acf2a 31dabab2080fe1d6e94db59aaa477f1cf4479b5c
gh pr checks 14421 --repo boykopovar/AnyPS5Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | No whitespace or convention errors |
| Main merge preview | passed | Clean against current main |
| Export audit | passed | Warnings are test callback declarations, not missing exports |
| Test registration | passed | CMake registers the added tests |
| Hosted checks | passed | Windows and relinker jobs passed; several specialized jobs skipped |
| Local build and CTest | not run | No local build |
| Negative control | not run | Requires exact-head C++ test binaries |
Raw check output
Precheck output:
Head: `31dabab2080fe1d6e94db59aaa477f1cf4479b5c`
Base: `b28bb772177768f99874ca38f6252cd0139acf2a`
Head unchanged at completion: **True**
Base unchanged at completion: **True**
| diff_check | passed | `git diff --check b28bb772177768f99874ca38f6252cd0139acf2a...31dabab2080fe1d6e94db59aaa477f1cf4479b5c` |
| conventions_and_pr_template | passed | `All conventions checks passed.` |
| live_dependency_and_mergeability_check | passed | `PR #14421: clean against current base` |
The ABI helper returned passed_with_warnings; all 23 warnings were for callback definitions used by the added tests. The test-quality heuristic warned that four C++ test sources may be unregistered; direct inspection of the CMake registration confirmed they are included.
Hosted checks:
conventions pass 28s https://github.com/boykopovar/AnyPS5/actions/runs/41486557045/job/125418359440
eligibility pass 5s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125418371035
labels pass 13s https://github.com/boykopovar/AnyPS5/actions/runs/41486557134/job/125418358229
library-portability pass 1m53s https://github.com/boykopovar/AnyPS5/actions/runs/41486561315/job/125418368677
relinker (macos-latest, clang++) pass 6m5s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424852988
relinker (macos-latest, clang++, x86_64) pass 6m47s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424853334
relinker (ubuntu-latest, g++) pass 4m3s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424852986
relinker (windows-latest, g++) pass 8m6s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424853191
edge-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424855834
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424854870
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424855792
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424855623
relinker-asan pass 2m23s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424852850
report pass 1m27s https://github.com/boykopovar/AnyPS5/actions/runs/41486561258/job/125418368226
windows pass 12m23s https://github.com/boykopovar/AnyPS5/actions/runs/41486561494/job/125424852634
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
| Returned entity mirrors retain independent contents and the correct owner document | No source mutation was made | Not run locally; no test currently covers these assertions |
Findings
Blocking
- Entity results alias one temporary object.
SyncHostEntityToGuestrewrites the samethread_local GuestEntityfor every call. A caller that saves the result ofxmlGetPredefinedEntity("amp")and then looks up"lt"receives a pointer whose fields now describe"lt"; a document entity result is overwritten the same way. Keep a stable guest mirror for each host entity for the relevant entity lifetime and add a regression that retains two lookup results and checks both after the second lookup. - Document entities lose their owner. The mirror unconditionally sets
GuestEntity::docto null. libxml2 associates document entities with their owning document, and consumers reading this field through the guest ABI will see a different relationship. Map the hostxmlDocPtrto itsGuestDocand preserve that pointer in the stable mirror; test a document entity'sdocfield.
Non-blocking
- The author reports that a maintainer decision is still pending on retaining zlib's required source-license notice alongside the repository's no-code-comments rule. Keep the required license notice intact while that policy question is resolved.
Sources
- Reused entity mirror and null
doc: Xml.cpp at the reviewed head. - Both exports return that mirror: Xml.cpp at the reviewed head.
- libxml2 2.9.14 returns addresses of static predefined entities and pointers from the document entity table: pinned entities.c, document lookup.
xmlAddEntitysets document entities'doctodtd->doc(same pinned source). - Existing tests do not retain and compare multiple lookup results: XML tests, document-entity test.
Not verified
- No local build, CTest, Windows/macOS build, GPU, or console run was performed. Hosted CI results above are separate and tied to this exact head.
- The zlib license-notice policy decision remains pending in the PR discussion.
To get this to a good state
Use stable guest entity mirrors, preserve each document entity's owning GuestDoc*, and add the two regression cases above. On the new head I will rerun the precheck, diff check, and hosted-check inspection listed under What I ran, then run the focused XML CTest if an exact-head build is available.
…st mirror and keep its document
|
@MarcZX10 thanks. b75abdf fixes both blocking items. 1. Entity results aliasing one temporary.
The same entity looked up twice returns the same pointer, and different entities never share one. 2. Document owner. Test.
Two negative controls:
How I tested: GCC 15 on Windows, Release with tests on. On the zlib notice: it stays in place, unchanged, until the maintainers decide on the policy. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, no blocking findings
The new per-entity mirrors and regression tests address the prior aliasing and missing-document-owner findings; hosted Windows and several platform jobs are still running.
What I ran
- Re-checked head
b75abdf6b120847836beea01fbf91f6fd52cf0c3against the PR-recorded baseb28bb772177768f99874ca38f6252cd0139acf2a; compared the three changed files since my previous review at31dabab2080fe1d6e94db59aaa477f1cf4479b5c. The precheck's current-main merge preview was clean againstd1ae628bc2f62459cce1e9872ba5632c938c71f2. - Environment: Linux x86-64, Python 3.11.2, GCC 12.2.0. No build or compiler invocation was run.
- Commands and inspections:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14421
git diff --check b28bb772177768f99874ca38f6252cd0139acf2a...b75abdf6b120847836beea01fbf91f6fd52cf0c3
git diff 31dabab2080fe1d6e94db59aaa477f1cf4479b5c..b75abdf6b120847836beea01fbf91f6fd52cf0c3 -- core/libs/prx/libSceNKWebKitRequirements/Xml.cpp core/libs/prx/libSceNKWebKitRequirements/tests/Xml.cpp docs/dev/debt/libSceNKWebKitRequirements.md
git show b75abdf6b120847836beea01fbf91f6fd52cf0c3:core/libs/prx/libSceNKWebKitRequirements/CMakeLists.txt
gh api repos/boykopovar/AnyPS5/commits/b75abdf6b120847836beea01fbf91f6fd52cf0c3/check-runsResults
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | No whitespace or convention errors. |
| Merge preview | passed | Clean against current main. |
| Entity regression registration | passed | The XML test remains registered through the CMake topic loop. |
| ABI audit | not applicable | No export declarations changed in this update. |
| Hosted checks | not run | Seven passed; four pending; three skipped. |
| Build and runtime tests | not run | No local build was performed. |
Commands and unedited outputs
Precheck command output:
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14421-b75abdf6b120/report.md
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14421-b75abdf6b120/report.json
Conclusion: checks failed
The precheck report says diff_check and conventions_and_pr_template passed, and the live merge preview passed. Its ABI and test-quality helper invocations failed because those scripts are absent from the isolated PR snapshot. The update does not alter export declarations; the full-diff lexical ABI audit emitted 23 warnings and is not treated as a correctness certificate.
The raw git diff --check output was empty (exit status 0).
Hosted check output at the final refresh:
[{"conclusion":"skipped","name":"write-watch","status":"completed"},{"conclusion":"skipped","name":"register-context","status":"completed"},{"conclusion":"skipped","name":"linux","status":"completed"},{"conclusion":"skipped","name":"edge-watch","status":"completed"},{"conclusion":null,"name":"relinker (macos-latest, clang++, x86_64)","status":"in_progress"},{"conclusion":"success","name":"relinker (macos-latest, clang++)","status":"completed"},{"conclusion":"success","name":"relinker (ubuntu-latest, g++)","status":"completed"},{"conclusion":null,"name":"relinker (windows-latest, g++)","status":"in_progress"},{"conclusion":null,"name":"windows","status":"in_progress"},{"conclusion":"success","name":"relinker-asan","status":"completed"},{"conclusion":"success","name":"eligibility","status":"completed"},{"conclusion":"success","name":"library-portability","status":"completed"},{"conclusion":"success","name":"report","status":"completed"},{"conclusion":"success","name":"conventions","status":"completed"},{"conclusion":null,"name":"labels","status":"queued"}]
Negative control
Not run: the added C++ regression needs an exact-head test binary, and no safe no-build mutation path was available.
Findings
Blocking
- None in the three-file update. Each returned entity now has a stable mirror keyed by its host entity; document-owned mirrors are discarded with their document, and the new test checks distinct entities, repeat lookup identity, preserved fields, and
GuestEntity::doc.
Non-blocking
- The PR depends on #14374, which remains open at head
3979b705f3b87d75827d036a587be8c37bc6f148; that current dependency head is not included in this branch. The stack preview is clean against currentmain, so this is not a conflict finding. Defer dependency integration until #14374 lands.
Sources
- Stable guest entity mirrors and owner mapping at the reviewed head
- Document cleanup releases entity mirrors
- Regression for multiple predefined and document entities
- XML test target registration
- Open zlib dependency at its current head
Not verified
- No local build, CTest, Linux runtime, Windows runtime, sanitizer, GPU, or console run was performed. The author-reported host-harness test for the entity mirrors is not independently reproduced.
- At the final refresh, Windows, Windows relinker, macOS x86-64 relinker and labels were pending; the Linux/macOS relinker and ASan jobs passed, and three specialized jobs were skipped.
- Dependency #14374 is still open; the current dependency head is not included in this stacked branch.
To get this to a good state
No blocking code change is needed in the reviewed update. Let the pending hosted checks finish and keep the #14374 stack relationship until that dependency lands. If this head changes, I will rerun the same no-build checks and review the new delta.
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (13, not counted against this PR)Validation IDs already present on main (4 IDs in 758 tests)Skipped (59)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
Implements the 60 libjpeg, libxml2 and libpsl exports of libSceNKWebKitRequirements, stacked on #14374 (zlib). Only the last five commits are new here; the three before them are #14374.
Jpeg.cpp): the decompression API (jpeg_CreateDecompress,jpeg_std_error,jpeg_read_header,jpeg_start_decompress,jpeg_read_scanlines,jpeg_finish_decompress,jpeg_save_markers,jpeg_resync_to_restart, …) over the libjpeg-turbo already in3rdparty/libjpeg-turbo. Guestjpeg_decompress_struct,jpeg_error_mgr,jpeg_source_mgrand saved markers are LP64 mirrors (include/GuestJpeg.hpp, sizes fixed bystatic_assert) copied to and from the host objects around each call. Guest source-manager, marker and error callbacks run throughAPS5_VABItrampolines. A null object, or one not created byjpeg_CreateDecompress, throwsstd::invalid_argument.Xml.cpp): parser contexts, push and memory parsing, SAX handlers, documents, entities, error access and output buffers over libxml2 2.9.14, a new submodule at3rdparty/libxml2built from source bycmake/LibXml2.cmakewithout iconv, ICU, HTTP, FTP or catalogs. Guest-visible structures are LP64 mirrors (include/GuestXml.hpp); SAX, input and output callbacks go through trampolines.Psl.cpp):psl_builtin,psl_load_file,psl_is_public_suffix,psl_registrable_domain,psl_unregistrable_domain,psl_is_cookie_domain_acceptable,psl_str_to_utf8lower, the builtin metadata getters and the rest of the 0.21.5 API, over libpsl 0.21.5, a new submodule at3rdparty/libpsl(with its pinnedlistsubmodule) built bycmake/LibPsl.cmake. The builtin table is generated at build time from the pinned list.The 60 stubs are removed from
Export.cppand their parameterless declarations inIdentified.hppare replaced by the real prototypes. 95 entries stay stubs (63 pending inventory exports, mostly SQLite, plus the 32 additional callable exports). Technical debt: pending counts updated and one Functional entry per library (no console verification,error_exithandling, libxml2 build options, libpsl snapshot).Tested
Windows 11, host harness built with the Qt MinGW 13.1 toolchain:
tests/Jpeg.cpp: decodes a JPEG encoded by libjpeg-turbo through a guest source manager (init_source,fill_input_buffer,skip_input_data,resync_to_restart,term_sourceas guest callbacks), checks dimensions, components, a saved APP1 marker and every pixel against host libjpeg output, then decodes a progressive image in buffered-image mode (jpeg_has_multiple_scans,jpeg_consume_input,jpeg_input_complete,jpeg_start_output,jpeg_finish_output), and drives a guesterror_exitthroughJERR_NO_SOI;tests/Xml.cpp: strings and dictionary, chunked push parsing with guest SAX2 callbacks, error retrieval through guesterror/serrorandxmlCtxtGetLastError, memory parsing with predefined and document entities, external entities loaded through two distinct guest input handlers (including a registration made while one stream is open and registrations rejected by a full table), and output registration and rejection;tests/Psl.cpp: builtin list queries (public suffix, registrable and unregistrable domain, cookie domain, counts, metadata),psl_str_to_utf8lower, andpsl_load_file,psl_load_fpandpsl_lateston a list the test writes, including wildcard and exception rules and their counts;Not covered by a test: guest output callbacks being called (no export of this library opens an output buffer).
Export.cpp,Zlib.cpp,Jpeg.cpp,Xml.cppandPsl.cppcompile with the realAPS5_VABI.tools/check_conventions.pyreports 0 errors (the one warning is the zlib entry from #14374) andtest_public_headers.py --audit-onlypasses. A full CMake configure is not possible here (missing SDL2/ffmpeg submodules), so the guest build and CTest run are left to CI.pr_overlap.py:.gitmodulesis shared with #1835, #5021, #9855 and #13578, which add other submodules; #14212 only reformats this debt file. None implements these functions.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)