Repository navigation
Conversation
|
Independent Windows check of
Exact tool/test blobs were extracted from Git; the workspace checkout was unchanged. Python/synthetic files only: no title replay, native build, CMake/CTest, GPU or Linux/macOS run. Composition with #5571 or current main was not tested; the overlap still needs handling separately. AI-assisted testing and reporting: OpenAI Codex. |
thepoppfish
left a comment
thepoppfish
left a comment
There was a problem hiding this comment.
Reviewed exact head 35bf15c510cb50ee8152d911c686e5eccdb95c15. I found two additional interactions between declared aliases and the existing filename matcher; details are in the inline comments.
Independent Windows 11 build 26200 / Python 3.14.7 / Git 2.55.0.windows.3 validation:
python -m unittest discover -s tools/tests -p test_import_audit.py -v: 127 tests run in 16.019 s; 124 pass and 3 skip (two POSIX permission tests and the symlink test, which lacks the Windows privilege on this host).- Conventions against main
2742004ed28f765c7147db0e0269ff5868e9eb6fandgit diff --check origin/main...HEADpass. - Additional synthetic checks: requesting
Declared.prxresolves with the physicalDeclared.prxprovider and two unrelatedDeclaredidentities. Changing only the requested suffix to.sprx,.suprx, or.debug_prxchanges exit 0 to exit 2. The same four cases all exit 0 with current main's tool. Failed PR cases preserve the existing report. - With a PE built library selecting Windows matching, an SCE identity
Declareddoes not resolvedeclared.prxby itself (exit 1). Adding the validDeclared.prxreference makes both resolve (exit 0). A SONAMEDeclared.prxalso resolves a case-mismatched request on this head. The relinker folds physical filenames infindGuest, whilematchesIdentitycompares declared identities exactly; this result should not depend on another registry entry.
Replay from the exact PR checkout, using its existing synthetic-image helpers:
from pathlib import Path
import json, sys
sys.path.insert(0, str(Path("tools/tests").resolve()))
import test_import_audit as t
def probe(requests, physical, tag, declared, windows=False, duplicate=False):
work = t.Workspace()
try:
(work.libs / "libc.prx").write_bytes(t.build_pe([]) if windows else t.exporting())
names = [] if duplicate else [(tag, declared)]
(work.modules / physical).write_bytes(t.build_elf(
[("AAAAAAAAAAA", 1, t.GLOBAL_FUNCTION)], module_names=names))
if duplicate:
for filename in ("first.prx", "second.prx"):
(work.modules / filename).write_bytes(t.build_elf([], module_names=[(tag, declared)]))
registry = work.root / "game.registry.json"
t.write_registry(registry, [("AAAAAAAAAAA", name) for name in requests])
registry.with_name("game.needed.json").write_text(json.dumps(requests), encoding="utf-8")
code, _, error = work.run(registry)
print(requests, "exit", code, error.strip())
finally:
work.close()
for suffix in (".prx", ".sprx", ".suprx", ".debug_prx"):
probe(["Declared" + suffix], "Declared.prx", 0x61000043, "Declared", duplicate=True)
for requests in (["declared.prx"], ["Declared.prx", "declared.prx"]):
probe(requests, "Provider.prx", 0x61000043, "Declared", windows=True)
probe(["declared.prx"], "Provider.prx", 14, "Declared.prx", windows=True)The relinker comparison was source inspection of GuestModuleBuilder.cpp at this head, not a native relinker execution. No C++ build, game, console or GPU test was run. The existing #7454-first order and its planned normalization remain separate from these findings.
AI-assisted review and testing: yes (OpenAI Codex).
| return imports, {name: sorted(files) for name, files in exports.items()} | ||
| requested = set(needed_names) | {name for _, needed in imports.values() for name in needed} | ||
| for name in sorted(requested): | ||
| if name in aliases: |
There was a problem hiding this comment.
[P2] Resolve existing filename aliases before rejecting identity ambiguity
This exact-name guard bypasses the existing suffix matcher. With a physical Declared.prx, two other modules declaring identity Declared, and a registry plus .needed.json requesting Declared.sprx, current main exits 0 but this head exits 2 with ambiguous guest module identity Declared.sprx. .suprx and .debug_prx reproduce it too; the exact .prx control passes. The relinker installs the filename suffix alias in neededAliases before identity resolution (GuestModuleBuilder.cpp:85,146 at this head). Please preserve that filename precedence before checking identity collisions and add a cross-suffix regression; unrelated identities should not invalidate an already-resolved physical module.
| module_imports, module_exports, modules = read_modules(args.modules, set(needed) | {library for _, library, _ in imports if library}) | ||
| host_modules = set(module_imports) | ||
| host_modules |= {name.removesuffix(".guest.prx") for name in host_modules if name.endswith(".guest.prx")} | ||
| modules |= host_modules |
There was a problem hiding this comment.
[P2] Keep declared aliases out of Windows filename case folding
The returned alias set is later wrapped in ModuleNames(..., fold=bool(windows)), so declared names inherit filename case folding. With a PE built library and Provider.prx declaring SCE identity Declared, a request for declared.prx correctly exits 1 by itself, but adding a valid Declared.prx reference makes both imports classify as module and the audit exit 0. A SONAME Declared.prx similarly accepts declared.prx. In the relinker, findGuest folds physical filenames only and matchesIdentity compares identities exactly (GuestModuleBuilder.cpp:88,140). Please retain exact matching for declared aliases separately from physical filename matching; adding another valid reference must not hide the unresolved import.
|
I prepared a tested follow-up for the two findings in my review:
Windows 11 build 26200, Python 3.14.7:
The follow-up is only 2 files, 54 insertions/15 deletions beyond the integration merge. It does not include #7454 or claim to settle that dependency's pending normalization; the agreed #7454 → #6171 order still applies and the combined result needs revalidation after that lands. The additional native converter/Windows synthetic-program checks below provide execution evidence for these matching rules. No title, console, Linux/macOS executable run or hosted build is claimed. If useful, the branch can be fast-forwarded from your current unchanged head: git fetch https://github.com/thepoppfish/AnyPS5 codex/import-audit-identity-followup
git merge --ff-only FETCH_HEADAdditional native comparison, with the fix commit unchanged: 12 paired audit/native conversion cases pass (six cases for Linux output, six for Windows output). The synthetic cases use the repository's actual
Native tool: the actual relinker-only Release build from Native comparison replaySave this driver outside the checkout. Adjust its from pathlib import Path
import hashlib
import json
import subprocess
import sys
import tempfile
work = Path(__file__).resolve().parent
repo = work / 'import-audit-identity-followup'
tool = work / 'build-relinker-10912/core/relinker/relinker.exe'
sys.path.insert(0, str(repo / 'core/relinker/relinker/tests'))
from test_guest_module_identity import declared_provider, executable
from test_guest_module_directories import needed_libraries
sys.path.insert(0, str(repo / 'tools'))
import import_audit
results = []
with tempfile.TemporaryDirectory(prefix='native-audit-', dir=work) as directory:
root = Path(directory).resolve()
assert root.is_relative_to(work.resolve())
cases = [
('physical-suffix', 'Declared.sprx', 'Declared', (), {
'Declared.prx': declared_provider(22, ('Unrelated',)),
'first.prx': declared_provider(11, ('Declared',), symbol='first'),
'second.prx': declared_provider(33, ('Declared',), symbol='second')}),
('identity-correct', 'Declared.prx', 'Declared', (), {
'Provider.prx': declared_provider(22, ('Declared',))}),
('soname-correct', 'Declared.prx', 'Declared', (), {
'Provider.prx': declared_provider(22, (), soname='Declared.prx')}),
('identity-wrong-with-correct', 'declared.prx', 'declared', ('Declared.prx',), {
'Provider.prx': declared_provider(22, ('Declared',))}),
('soname-wrong-with-correct', 'declared.prx', 'declared', ('Declared.prx',), {
'Provider.prx': declared_provider(22, (), soname='Declared.prx')}),
('soname-suffix-with-correct', 'Declared.sprx', 'Declared', ('Declared.prx',), {
'Provider.prx': declared_provider(22, (), soname='Declared.prx')})
]
for windows in (False, True):
for label, request, identity, extra, providers in cases:
case = root / f'{label}-{windows}'
modules = case / 'sce_module'
modules.mkdir(parents=True)
for filename, image in providers.items():
(modules / filename).write_bytes(image)
source = case / 'input.elf'
source.write_bytes(executable(request, module_name=identity, extra_dependencies=extra))
output = case / ('output.exe' if windows else 'output.elf')
converted = subprocess.run([str(tool), *(['--windows'] if windows else []), str(source), str(output)],
capture_output=True, text=True, timeout=30)
assert converted.returncode == 0, (label, windows, converted.stderr)
item = {'windows': windows, 'case': label, 'conversion': converted.returncode, 'error': converted.stderr.strip()}
_, _, names = import_audit.read_modules([modules], (request, *extra), windows=windows)
item['audit_matches'] = {name: name in names for name in (request, *extra)}
valid = label in ('physical-suffix', 'identity-correct', 'soname-correct')
expected = {request: valid, **{name: True for name in extra}}
assert item['audit_matches'] == expected, item
if converted.returncode == 0:
if windows:
ran = subprocess.run([str(output)], capture_output=True, text=True, timeout=30)
item.update(runtime_exit=ran.returncode, runtime_error=ran.stderr.strip())
if valid:
assert ran.returncode == 22 and not ran.stderr, item
else:
missing = request.removesuffix('.sprx') + '.prx' if request.endswith('.sprx') else request
assert ran.returncode != 0 and 'Failed to load module:' in ran.stderr and '\\libs\\' + missing in ran.stderr, item
else:
item['needed'] = needed_libraries(output.read_bytes())
unresolved = request.removesuffix('.sprx') + '.prx' if request.endswith('.sprx') else request
assert (unresolved in item['needed']) is not valid, item
print(json.dumps(item), flush=True)
results.append(item)
assert len(results) == 12
report = {'audit_commit': subprocess.check_output(['git', '-C', str(repo), 'rev-parse', 'HEAD']).decode().strip(),
'relinker_commit': subprocess.check_output(['git', '-C', str(work / 'review-10912'), 'rev-parse', 'HEAD']).decode().strip(),
'relinker_sha256': hashlib.sha256(tool.read_bytes()).hexdigest(), 'cases': results}
(work / 'native-import-audit-parity.json').write_text(json.dumps(report, indent=2), encoding='utf-8')
print('12 paired audit/native conversion cases passed; all six generated Windows programs checked.')I have left the existing review findings open on this PR head and opened no competing PR. AI-assisted: yes, OpenAI Codex. Control replayOn the published branch, run from pathlib import Path
import importlib.util
import io
import subprocess
import sys
import tempfile
import unittest
work = Path(__file__).resolve().parent
repo = work / 'import-audit-identity-followup'
sys.path.insert(0, str(repo / 'tools/tests'))
import test_import_audit as tests
current = (repo / 'tools/import_audit.py').read_text(encoding='utf-8')
baseline = subprocess.check_output(['git', '-C', str(repo), 'show',
'048a9923ed6ff26516bd80a08322fd96e4a9bc22:tools/import_audit.py']).decode('utf-8')
priority = 'if name in aliases or normalized in folded_files or (file_suffix and name in filenames):'
namespace = 'self.stems = {module_stem(name, fold) for name in names}'
assert current.count(priority) == current.count(namespace) == 1
controls = [
('integrated baseline', baseline, 16),
('without filename precedence', current.replace(priority, 'if name in aliases:'), 6),
('without exact alias namespace', current.replace(namespace, 'self.stems = {module_stem(name, fold) for name in self}'), 10)
]
original = tests.import_audit
try:
with tempfile.TemporaryDirectory() as directory:
for index, (label, source, failures) in enumerate(controls):
path = Path(directory) / f'control_{index}.py'
path.write_text(source, encoding='utf-8')
spec = importlib.util.spec_from_file_location(f'import_audit_control_{index}', path)
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
tests.import_audit = module
suite = unittest.TestSuite(tests.ImportAuditTests(name) for name in (
'test_exact_module_filename_precedes_other_module_identities',
'test_declared_aliases_do_not_inherit_windows_filename_matching'))
log = io.StringIO()
result = unittest.TextTestRunner(stream=log).run(suite)
(work / f'import-audit-followup-control-{index}.log').write_text(log.getvalue(), encoding='utf-8')
assert len(result.failures) == failures and not result.errors, (label, log.getvalue())
print(f'{label}: {len(result.failures)} expected regression failures, no setup errors', flush=True)
finally:
tests.import_audit = original |
|
Updated the main-only #10933 fix after the UTF-8/BOM import-audit changes merged: 83dda8a07a2, on Windows/Python3.14.7:139 pass/3 skip in18.264s; current-main negative6 failures, exact-filename/absent-stem controls6/2, zero errors. Repeated actual native Linux/Windows registry comparisons still show main's false module/exit0 and the fix's correct absent/exit1; the generated Windows program explicitly fails on bare Declared, matching the fix. Full replay is in the latest #10933 revision reply. Conventions/whitespace pass, fork branch pushed and clean; no new product/hosted/game claim. The physical-filename/exact-declared-alias distinction and optional identity follow-up18f2abb6aa remain as previously discussed. This rebase includes no unmerged #6171/#7454 features and does not revalidate the historical6c2be569ac integration. I still propose this independent main fix before #6171; the order remains unagreed. #10942 is closed and unmerged, so carrier coordination is also still needed; no competing PR is open while #10864 occupies my author slot. A fresh canonical overlap scan is required before publication. AI-assisted: yes. |
|
I refreshed the existing integration aid on main Could we settle one carrier for #10933: would you prefer the main-only bare-filename fix to land first, or to carry it here before the identity changes? I have not opened a competing PR. The aid still covers only #7454's published simple $ORIGIN handling, not its unpushed directory-bearing update. |
|
Thanks @thepoppfish — I independently checked your exact
The offered integration retains my unchanged The current-main previews pass conventions and whitespace checks against the pinned main. No fresh Windows, macOS, console or native-game execution is claimed. Your correction and authorship are preserved in the local previews; nothing has been pushed from this validation. AI-assisted validation and reporting: OpenAI Codex. |
|
Thanks for the independent checks. I will keep #7454 before #6171. The existing integration aid now also carries the reporter's host-sidecar correction from #10933: declared aliases cannot satisfy a remaining physical host dependency. My 155-method audit suite passes on Windows and WSL with the expected platform skips. This still includes only #7454's published simple-prefix handling. Your directory-bearing cases and the separate #10933 carrier decision remain pending; I have left your draft and branch unchanged. |
|
The #11499 author has confirmed #11499-first. I will retain its whole-entry traversal and partial-entry handling when refreshing the existing alias integration aid, alongside SONAME/SCE identity collection. The #7454 → #6171 order remains unchanged; the proposed #11581-first carrier still awaits confirmation. This records the new agreement, without claiming a combined test run or changing your branch. |
|
Withdrawing this draft for now. It remains dependent on #7454; the branch and its validation evidence are preserved. I am using the available PR slot for a separate FreeType dependency security update. |
What
Correct a false failing import audit for Middle Ages Ping Pong, PPSA28319 01.000.000. Its
Il2CppUserAssemblies.prxdeclares the SCE identityIl2cppUserAssemblies. The relinker resolves that identity, but the audit reports four absent imports and a missing module.Read ELF SONAMEs and SCE module identities using the relinker's exact-name precedence and suffix rules. Preserve ambiguity checks, import counts and physical filename requirements for host dependencies. This corrects preflight diagnostics; the title already runs.
Merge order: #7454, then #6171, agreed with #7454's author. The prepared identity-side normalization will be applied with the rebase after #7454 lands; its implementation is not bundled here.
Tested
Head
35bf15c51, rebased cleanly onto synced main8727ddb29. The tool, tests and documentation are byte-for-byte identical to the earlier October 20 head98596149e; fresh Linux checks and the saved title audit pass again. The contribution combines declared identities with main's newModuleNamesfilename-suffix and Windows case matching. It preserves unscoped guest-import lookup, NID disambiguation normalization, PE support and the Windows host-importer set. Host lookup still receives physical module filenames rather than guest identity aliases.2626e5ee7registries/libraries and current source classification, main's tool exits 1 with four absent imports; the PR exits 0 with none. Both retain 2,355 references, 1,052 unique imports and eight stub classifications. No missing host dependencies. This reruns the audit, not the game or native build.98596149e, whose audit tool/test blobs exactly match this head; Windows was not rerun for this rebase. Fresh local Linux validation also passes all six subprocess CLI checks. The seven regression methods produce 83 failed subcases and zero errors against main's tool. No upstream native CI is claimed.11840ad87and our74c5e7029passed 122 Linux tests with one skip, plus four cross-feature methods. The author and another contributor report compatible combinations at their recorded pins. These are historical results; the combination will be revalidated after fix(tools): resolve $ORIGIN-prefixed converted module dependencies in import_audit #7454 lands on current main.Pinned evidence, CI results and replay patch. Historical runtime evidence remains main
2626e5ee7: rendered match and exit 0 after SIGINT at 45.47 s, RTX 5070/NVIDIA 595.99.02, SDL dummy audio. No full-playability, performance, console result or second affected title is claimed. Earlier independent Windows validation applies to the original revision.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)