Repository navigation
fix(relinker): reject wrapping address32 wide-store splits - #14214
Gabrieljr42 wants to merge 3 commits into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The exact-head patch passes git diff --check, and the Ubuntu relinker and ASan jobs pass, but the mandatory current-main checklist item is unchecked and the exact-head conventions job fails.
What I ran
Head 819ad8ddd5b9f56b8d72cad12c6a6741a27e3785; PR base 8a4fc62318a4b73b6895bf20a024c625efeb7c3b. The local merge preview against that base was clean. Environment: Debian Linux x86-64, Python 3.11.2; no compiler or build was run.
git diff --check 8a4fc62318a4b73b6895bf20a024c625efeb7c3b...819ad8ddd5b9f56b8d72cad12c6a6741a27e3785
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14214 --output-dir /tmp/anyps5-precheck-14214
gh pr checks 14214 --repo boykopovar/AnyPS5
gh run view 41255189754 --repo boykopovar/AnyPS5 --job 124696753518 --log-failedResults
| Check | Result |
|---|---|
| Diff whitespace | Passed; git diff --check emitted no output. |
| Current-main merge preview | Passed; clean against 8a4fc62318a4b73b6895bf20a024c625efeb7c3b. |
| Conventions and PR template | Failed: required “Based on current main; no other open PR implements the same functions” box is unchecked. |
| Ubuntu relinker | Passed on this head. |
| Relinker ASan | Passed on this head. |
| Windows and macOS relinker | Pending at the last check. |
| Product Linux/Windows and specialized jobs | Skipped by the workflow. |
Unedited conventions output
(pull request): error: [pr-template] Fill in the pull request template: unchecked: Based on current `main`; no other open PR implements the same functions
1 findings, 1 errors; GitHub shows at most 10 error annotations, the job summary lists all
Findings
Blocking
The PR template leaves the required current-main/overlap assertion unchecked, so repository conventions fail on this head. The description says its validation used b08c539 while current main is 8a4fc623; please recheck the assertion against the current base and update the PR description so the required checklist item is completed truthfully.
Non-blocking
The documented rejection of safe runtime 32-bit base/index addresses is an explicit limitation of this conservative lowering and is disclosed in the PR description and debt entry.
Not verified
No C++ build, CTest, Rosetta execution, title run, or GPU/console test was run. The Windows and macOS relinker jobs were still pending.
To get this to a good state
Please update the required checklist evidence and resolve the conventions failure. I will rerun the same checks on the new head.
|
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.
|
|
Merged current The merged relinker-only Release build passed, as did the expanded seven focused tests plus The description/checklist now records this current base/head and evidence. No new title, GPU or Rosetta execution is claimed. AI-assisted: yes (Codex). Additional manual overlap check for #14085 at Its only shared-file edit corrects Windows-import inventory wording, separate from this PR's wide-store debt entry; it changes no relinker code or converter tests. No duplicate implementation, dependency, or added conflict was found. This manually covers the ninth bot-listed comparison at those exact SHAs; it does not turn the bot's unverified result into an automatic pass. No code changes were needed. AI-assisted: yes (Codex). |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The updated head includes the current-base evidence and passes conventions; the wide-store guard and regression cases are unchanged in intent.
What I ran
Reviewed head 9d984a07a07206cf4f0da30fe1ecd35cb66e16aa against PR base cc26ab44380db3cd7b53715b84f13945227ce28b; the merge preview onto current main e3a62678dd92332218d041df4693c7675f43018e was clean. Environment: Debian Linux x86_64, Python 3.11.2, Git 2.39.5. I did not build locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14214
git diff --check cc26ab44380db3cd7b53715b84f13945227ce28b...9d984a07a07206cf4f0da30fe1ecd35cb66e16aaResults
| Check | Result |
|---|---|
| Diff and conventions/template checks | Passed. |
| Current-main merge preview | Clean; no declared dependencies. |
| Hosted relinker CI for this head | Ubuntu, Windows and both macOS relinker jobs plus ASan passed. Product Linux/Windows and specialized jobs were skipped. |
| Author-reported validation | Relinker-only Release build and 8/8 focused tests passed on Windows 11; not independently reproduced. |
| Test-quality helper | Not run: helper script is absent from the isolated PR checkout. The test assertions and registration were inspected directly. |
Unedited conventions output
All conventions checks passed.
Findings
Blocking
None. The previous checklist failure is fixed, and the same two-line conservative rejection remains covered by boundary and control cases.
Non-blocking
The guard intentionally rejects some safe runtime base/index addresses as well; the PR documents this limitation and leaves full effective-address lowering for later.
Sources
- Address-size guard at the reviewed head
- Address-wrap regression cases at the reviewed head
- Documented unsupported lowering
Not verified
No local build or CTest was run. Product Linux/Windows CI, Rosetta execution, titles, and console behavior were not verified in this review.
What
--to-rosettacan turn a valid67 c5 fe 7f 01(VMOVDQU [ecx], ymm0) at0xfffffff0into a 16-byte store followed by a write to address zero. The original 32-byte operand extends past 4 GiB; recomputing the upper half with 32-bit addressing wraps it. This remains possible with zero/small-displacement base/index forms after #13046.Reject eligible address-size-32 unaligned 256-bit stores with a runtime base or index, and record the unsupported lowering. This also rejects safe runtime addresses; an unexecuted matching instruction can stop conversion. Full support needs one effective-address calculation and a full-width
+16, which is not implemented. No affected-title frequency or Rosetta execution is claimed. Intel mode, narrower/aligned stores and the existing address64/absolute/EIP-relative handling are preserved by regression checks.Current
maincc26ab44380db3cd7b53715b84f13945227ce28bwas incorporated by a normal merge into validated/pushed head9d984a07a07206cf4f0da30fe1ecd35cb66e16aa. The original published head819ad8ddd5b9f56b8d72cad12c6a6741a27e3785remains an ancestor; no force-push was used. Both upstream main and the exact remote PR head were verified after the push.Tested
Windows 11 x86-64, WinLibs GCC 15.2.0, CMake 4.3.1, Ninja 1.13.2, Python 3.12. On the merged head above, the relinker-only Release configure/build passed. The seven focused conversion tests plus
code_data_scanningpassed 8/8 in 2.70 seconds, with no CTest-reported skips (amd64_only|to_rosetta|masked_store|guest_intel|to_intel|code_data_scanning). The extra scanner test covers the relevant upstream scanner changes. The guard and expanded converter test source remain byte-for-byte identical to the original published head: eight accepted address64 counterparts checked against independently assembled bytes, six narrower/aligned address32 controls and FS/GS absolute/EIP relocation controls.The fresh overlap checker fetched 1,692 open PR heads and probed 1,691 other branches, examining 34 related candidates. It reported no confirmed overlap and exited 2, because eight PRs already conflict with the base and share relinker debt: #5021, #7531, #10792, #10861, #11991, #12678, #12910 and #14212. Fresh substantive source and three-way merge reviews of all eight found no duplicate implementation or conflict added by this PR. A final open-head recheck found 1,694 open PRs; the three new/changed heads (#14219, #14218 and #12596) were also reviewed and affect unrelated GPU/input code. The current-main/overlap assertion is based on this fresh evidence, not the older pre-publication scan.
Conventions (including this completed PR template) and
git diff --checkpassed against the current base. The earlier native synthetic AVX probe showed the original instruction writing all 32 bytes across 4 GiB with guards intact, while bytes emitted by the compiled baseline decoder wrote 16 and faulted at zero; the separate seven-form rejection regression changed from failing to passing with the guard. No title, GPU, macOS or Rosetta execution; the full suite was not run, and platform-conditional C++ execution cases do not establish Rosetta behavior.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)