Skip to content

fix(relinker): align ELF64 section header tables - #10852

Merged
oneandonlydean merged 2 commits into
boykopovar:mainfrom
Mherberts:codex/fix-elf-section-table-alignment
Oct 26, 2026
Merged

oneandonlydean merged 2 commits into
boykopovar:mainfrom
Mherberts:codex/fix-elf-section-table-alignment

Conversation

@Mherberts

@Mherberts Mherberts commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

What

SectionHeaderTableBuilder writes the ELF64 section header table immediately after .shstrtab, so its file offset is usually unaligned. A synthetic conversion on main writes e_shoff = 0x82ba. The gABI data representation rules require natural alignment for ELF64 structures, including their 8-byte fields.

Align the buffer to 8 bytes before writing the table. In that conversion, e_shoff becomes 0x82c0; a byte comparison confirms that only e_shoff and six inserted zero bytes differ. The string table, section headers and preceding contents are preserved.

Extend the existing native regression test across all eight input-size residues. It checks alignment, minimal zero padding, exact string-table size and bounds, unchanged preceding contents and section offsets.

Tested

Windows x64, build 26200; WinLibs GCC 15.2.0 r7; Release, ANYPS5_RELINKER_ONLY=ON, BUILD_TESTING=ON, Python 3.13 with PYTHONUTF8=1.

  • The new regression fails against main e7e3e9c with ELF64 section header table is not 8-byte aligned, using the same GCC 15.2.0 toolchain. It passes with this change: 21 section range and alignment cases.
  • Review update at d15fe22: require sh_size == sizeof(Elfpatcher::kShStrTabBlob) for all eight residues. Rebuilt section_header_ranges_tests and reran ctest --test-dir build-verify -R '^section_header_ranges$' --output-on-failure: all 21 cases passed (0.13 s test, 0.18 s total). With GCC 15.2.0, a mutation setting shStrTabSize = shOff - shStrTabOff still passes the original test, while the stricter test fails with Section range validation failed. Convention checks against both origin/main and upstream/main (19fe176) passed. The full suite was not rerun for this test-only update.
  • Full CTest run at 03f3829 (before the assertion-only review update): 257/260 passed in 564.51 s; all relinker tests passed, including section_header_ranges (0.18 s). The three auxiliary failures were import_audit (temporary-file access denied on OneDrive), performance_compare (20 s timeout), and shader_cache_runtime_sources (180 s timeout). A serial CTest rerun with the default local temp directory passed import_audit in 14.27 s and performance_compare in 10.26 s. The shader-source helper also passed all nine Python tests when run directly with the default local temp directory (189.669 s), but that duration still exceeds its configured CTest timeout. These helper sources and their registration are unchanged from main; the full CTest run is not claimed as clean.
  • Synthetic input converts to Linux ELF, Windows PE and macOS Mach-O. The generated Windows executable runs and returns the expected exit code 42. Only Linux output uses this section-table writer; PE and Mach-O conversions provide smoke coverage.
  • python tools/check_conventions.py --base origin/main: passed.
  • python tools/check_test_registration.py build-verify: passed.
  • Overlap check: compared the changed paths of all 1,293 open PRs, then used tools/pr_overlap.py for relevant comparisons. None share either changed file. The local runner substitutes unauthenticated public REST pagination for the unavailable gh command and filters disjoint paths; no exports are changed.

No commercial title or GPU runtime was exercised.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 03f382934ee948d9e3261a8b51b3330738c58883. The production change aligns e_shoff to the required 8-byte boundary before recording it, and the new residue loop checks alignment, minimal zero padding, .shstrtab bounds, unchanged preceding bytes, section offsets and final table size. The diff is narrowly scoped to the section-table writer and its native regression test. The PR reports that this focused test fails on main and passes with the change, and documents the full CTest exceptions and reruns transparently. I could not independently run the test here because the available /marc/AnyPS5 checkout is not at this PR head and has no configured test build. Hosted checks currently pass conventions, intake and labels; no product test check is reported. Based on the source and evidence available, I found no specific blocking defect, but this is a comment-only review rather than an approval because I have not run the PR-head test.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 03f382934ee948d9e3261a8b51b3330738c58883 against current main e7e3e9c1984c293f06bff08428b8c89b7b932ac2.

The production change aligns the appended section-header table to 8 bytes before recording e_shoff, while preserving the string table and existing section data. The new residue cases check alignment, minimal zero padding, string-table bounds, preceding bytes, section offsets and final table size.

I independently compiled and ran the exact section_header_ranges source set from this PR with GCC 12.2/C++20: all 21 cases passed. As a negative control, I compiled the same PR test against main's SectionHeaderTableBuilder.cpp; it failed with the expected ELF64 section header table is not 8-byte aligned diagnostic. This is a focused native test, not the full relinker CTest suite. Full CMake configuration here is blocked by missing SDL2 development files. The PR reports its broader Windows validation, including the unrelated CTest failures and direct helper rerun.

Hosted conventions, intake and labels checks pass; product jobs are not reported as run. I found no blocking issue in this focused change.

AI-assisted review: yes.

@roboterspinne roboterspinne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 03f3829 merged onto upstream/main 63a863d (2026-10-25); the merge is clean.

Environment: Windows 11 Home 10.0.26200, Intel i9-12900H, WinLibs GCC 15.2.0 UCRT, CMake 4.3.1, Ninja 1.13.2, -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Release.

Results

  • Full relinker-only suite on the merged tree: 261 tests, 259 pass, long_paths skipped. The one failure is patch_advisor_unit. It fails the same way on plain main on this machine, because the temp path contains a space (#10667), and it is unrelated. section_header_ranges passes.
  • Counter-check: with main's SectionHeaderTableBuilder.cpp under this test, section_header_ranges_tests exits 1 with ELF64 section header table is not 8-byte aligned. With this branch it prints 21 section range and alignment cases passed. The eight tails give table-start residues 2, 3, 4, 5, 6, 7, 0 and 1 (the blob is 42 bytes), so every residue is covered.
  • End to end, I converted the test_syscall_number fixture to a Linux ELF with a main build and with this branch:
    • e_shoff moves from 0x82ba to 0x82c0, as in your description.
    • The bytes before the old e_shoff are identical, the 6 inserted bytes are zero, and the section header table is byte-identical.
    • .shstrtab keeps sh_offset/sh_size.
    • binutils readelf -S -W accepts both files without warnings.
  • SectionHeaderTableBuilder is built only on the Linux output path (Conversion.cpp:168), and WriteTable is the last step in LinuxElfPatcher.cpp:586. The in-repo readers of the output that I found take e_shoff from offset 0x28: test_linux_dynamic_strings.py:42, test_weak_imports.py:152, tools/tests/test_import_audit.py and EntryPointCollector.cpp:155. I found no size or golden-file comparisons, so the extra 0-7 bytes at the end are safe.
  • check_conventions.py --base upstream/main passes with this title and body. Since you are a first-time contributor, the hosted relinker jobs were skipped by the eligibility gate, so this run and yours are the only build evidence.

Notes, nothing to change here

  • No consumer I tried rejects main's misaligned table (binutils readelf, objdump -h, LIEF). LLVM's ELFFile::sections() does have an invalid alignment of section headers check (llvm/Object/ELF.h:1057), but it tests alignof(Elf_Shdr), and LLVM's ELF field types are declared with alignment 1 (ELFTypes.h:65), so it does not fire either. This is a spec-conformance fix (gABI, Data Representation: data have "suitable alignment from the beginning of the file"), not a fix for a reported consumer failure, which matches how you describe it.
  • The Windows PE and Mach-O conversions in Tested don't exercise this change, because only the Linux patcher uses the builder.

Not checked: Linux and macOS locally, and LLVM tools (none installed here; the LLVM point above comes from reading its source).

Apart from the nit on line 45, I found nothing to change. For disclosure: I have open relinker PRs (#10752, #10732, #10731, #10730, #10712, #10679, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390) plus #10667 on tools/tests/test_suggest_patches.py; none touches core/relinker/elfpatcher/src/general/ or core/relinker/elfpatcher/tests/. #10607, #10564 and #10402 change LinuxElfPatcher.cpp, which calls the builder, and each merges cleanly with this PR.

Written with AI assistance (Claude Code). I ran the build, the tests, the counter-check and the end-to-end comparison above on my Windows 11 machine.

const auto stringOffset = Io::ReadU64(bytes, alignedTable + 64 + 24);
const auto stringSize = Io::ReadU64(bytes, alignedTable + 64 + 32);
const auto stringEnd = stringOffset + stringSize;
Require(stringOffset == originalSize && stringSize != 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: stringSize != 0 lets a fix that counts the padding as part of .shstrtab pass. I linked this test against a builder that sets shStrTabSize = shOff - shStrTabOff after AlignBuffer. All 21 cases pass, while sh_size runs 48, 47, 46, 45, 44, 43, 42 and 49 over the eight tails instead of 42. The output is still a valid string table, so this is only a weaker assertion. Pinning it would close the gap:

Require(stringOffset == originalSize && stringSize == sizeof(Elfpatcher::kShStrTabBlob));

This needs #include <elfpatcher/general/ElfConstants.hpp>, because the test doesn't see the blob otherwise. With that include, the stricter test compiles, still passes on this branch, and fails against the padded-size builder.

@oneandonlydean oneandonlydean self-assigned this Oct 26, 2026

@roboterspinne roboterspinne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mherberts Thanks, checked d15fe22 merged onto main 204c674 (2026-10-26); the merge is clean. Windows 11, GCC 15.2, relinker-only Release.

  • Full ctest: 306 of 307 pass, including section_header_ranges; patch_advisor_unit fails the same way on plain main, and long_paths is skipped.
  • The padded-size mutant, which writes the .shstrtab header with shOff - shStrTabOff, now fails section_header_ranges; the test of 03f3829 still passed it. So the exact size is pinned.

That settles my nit; I found nothing else to change.

Written with AI assistance (Claude Code). I ran the checks above on my Windows 11 machine.

@oneandonlydean
oneandonlydean merged commit ec9256a into boykopovar:main Oct 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants