Repository navigation
Give the ppc64le resolver a real ABI frame header - #60
Conversation
Nemo-010
left a comment
There was a problem hiding this comment.
I reproduced the bug on main and verified this branch fixes it, including the regression test. Everything below is from running the code, not reading it.
The bug is real and the diagnosis is exact
glfwd_resolve_one's generated prologue on ppc64le:
mflr 0
std 31, -8(1)
stdu 1, -128(1)
std 0, 144(1) # <- 16(r1) of the CALLER = the old r5 save slotSo 144 - 128 = 16 lands on the caller's frame at offset 16, which is where the old std 5, 16(1) put r5. Confirmed against the actual compiler output, not just the ABI text.
Reproduced with an 8-long target under qemu-ppc64le 7.2 and a Debian ppc64el glibc 2.41 sysroot:
gl-fwd.c |
result |
|---|---|
main (3095c15) |
first=3 second=0 → FIRST CALL WRONG |
| this branch | first=0 second=0 → all passed |
The regression test does what it claims
I extracted the tgt.c / tgt-fwd.h / m.c this Makefile generates and ran them exactly as make gl-fwd-qemu-check would:
- against this branch:
all passed, exit 0 - against
main'sgl-fwd.cwith the same generated sources: exit 5, i.e.t_manyreturned nonzero on its first call
So the new case is a genuine regression test and not a test that happens to pass both ways. Putting t_many first in main() matters, and it is first.
Frame layout
I checked the arithmetic rather than trusting it:
- header 0..112, first saved register at 112 → no overlap with the callee's back chain / CR / LR / TOC slots
- highest used byte is 296 (LR at 288..296), frame is 304 → 8 bytes of slack, no overlaps anywhere
- 304 is 16-byte aligned, which the
blrequires
ELFv1 (BE ppc64) hunk
The body says this is edited the same way and not exercised, which is honest. I checked what I could:
- it compiles for
powerpc64-linux-gnu(zig) - the translation is faithful:
stdu 1,-304(1), regs from 112, LR at 288,addi 1,1,304
I could not run it. Debian has no ppc64 (BE) port any more, and ArchPOWER's ELFv2 glibc does not define the GLIBC_2.3/GLIBC_2.4 symbols zig's ELFv1 stubs want — the same ELFv1-vs-ELFv2 split as issue #11. So "a one-off run under qemu-ppc64 passed" is not something I can corroborate; I am not disputing it either, just flagging that it is unverified from here.
One observation on that hunk while reading it: ELFv1 never saved r11 in the first place (main has no std 11 in that body, and its ld 11, 16(3) is the descriptor load, not a restore). This branch keeps that behaviour unchanged, so the ELFv1 and ELFv2 paths now differ in whether they preserve r11. That is pre-existing rather than introduced here, and it may well be correct for ELFv1 where r11 comes from the descriptor — but the two bodies now look more similar than they behave, which is worth a comment if it is deliberate.
Minor
docs/building.md and docs/report/10-measured-versus-assumed.md both describe the new case accurately. No other files touched.
Not covered
Still nothing in CI runs gl-fwd-qemu-check — gates.yml builds and runs verify-artifacts.sh, which never executes a trampoline. This PR adds the case that would have caught the bug, but the target it lives in is still not wired into the matrix. That is a separate change and I would rather see it land than block this one, since the fix itself is correct and the AppImage consumers are currently broken on ppc64le.
|
Correction to my review, and it removes the one caveat I raised. I flagged the ELFv1 (BE ppc64) hunk as "unverified" and noted that ELFv1 and ELFv2 now differ in whether they preserve r11. That was wrong of me: ppc64 BE is not a supported target, dropped in #36 because its artefacts cannot load where they ship — Debian's So:
Which means the r11 observation in my review is about dead code, and my inability to run the ELFv1 path is not a gap in this PR's verification — there is nothing on that path to verify against a release. Two consequences for the PR itself:
Nothing else in my review changes. The ppc64le diagnosis, the frame arithmetic, the fix working on |
|
Strike point 2 from my previous comment — I checked and the docs are right, not misleading.
So the only real content of the correction is the first half: ppc64 BE is not a supported target, so the ELFv1 hunk in this PR is dead code for every architecture that ships, my "unverified" caveat about it was not a gap, and the r11 observation I raised was about code no release builds. |
|
Thanks for the review. One correction to the commit message, which I cannot amend without a push. The line "A one-off run under qemu-ppc64 passed with the change and failed without it" is broader than what was run:
Agreed that the ELFv1 hunk is not needed for the ppc64le fix and no shipped target builds it. It only keeps the two bodies in step. If you prefer, drop it. |
glfwd_resolve_asm saved the argument registers at 0..64(sp), which is the back chain, CR, LR and TOC save area plus the parameter save area. A C callee writes its caller's LR slot at 16(sp) and the TOC slot at 24(sp), so glfwd_resolve_one, which is not a leaf, overwrote the saved r5 and r6 on the first call through every slot. The first call of any GL entry point taking a third or fourth integer argument was handed garbage. Reported in issue 59 on the gtk3-demo ppc64le AppImage: the shipped gl-fwd.so exits 139 when preloaded, one built from this change stays alive. Not rerun here. Registers are now saved above the 96-byte callee area (32-byte ELFv2 header plus a 64-byte parameter area). The frame is 288 bytes. make gl-fwd-qemu-check gains a first-call test with eight live integer arguments; it fails on the old frame (ppc64le) and passes on the new one. The ELFv1 big-endian ppc64 trampoline block is removed. ppc64 is not built (see docs/limits.md), nothing ran that block, and a ppc64 ELFv1 compile now stops at the existing "needs a tail-jump trampoline" error.
a5dd48f to
b42238b
Compare
|
Re-verified against the new head New frame arithmetic. Header 0..96, first saved register at 96, highest used byte 280 ( Fix still works. 8-long target,
The PR's own regression test still discriminates. I extracted the The ELFv1 removal does what the message says. With a genuine ELFv1 compile ( which is the right outcome — an unsupported ABI now fails loudly at build time instead of shipping a body nothing runs. One note on your correction to the commit message: it is accurate and more precise than the original line, and I appreciate you pinning down that No objection to dropping the ELFv1 hunk, and no further comments from me. From the ppc64le side this is correct and I would merge it. |
Nemo-010
left a comment
There was a problem hiding this comment.
Approving. Verified on this head (b42238b), not just read:
mainsegfaults on the 8-long target underqemu-ppc64le; this head givesfirst=0 second=0- the new
t_manycase passes here (exit 0) and fails onmain(exit 5), so it is a real regression test - frame arithmetic: header 0..96, highest used byte 280, frame 288, no overlaps, 16-byte aligned
- a genuine
-mabi=elfv1compile now stops at the#error, which is the right outcome for an unsupported ABI
CI is green on all five architectures. Thanks for turning the ELFv1 block into a loud build failure rather than leaving an untested body in place — that is the difference between a shim that works and one that only looks like it does everywhere.
Both are in PROGRESS.md, and both were caught by asking what had been asserted without being run. 900a393 is a squashed commit with one parent, not the PR #60 merge commit. The property that matters is unchanged and is now stated as the property: the tree CI tested (62985da) is the tree the tag carries, so the commits differ and the trees do not. The gtk3-demo ppc64le reproduction came from issue #59 and the PR body, not from this session. What was measured here is that `make gl-fwd-qemu-check` fails on the pre-fix gl-fwd.c and passes on the shipped one, and that the published ppc64le-gl-fwd.so carries the 288-byte frame. Nothing here ran a GTK3 application, so the entry now says so instead of repeating the report as though it were a result.
glfwd_resolve_asm saved the argument registers at 0..64(sp), which is the
back chain, CR, LR and TOC save area plus the parameter save area. A C
callee writes its caller's LR slot at 16(sp) and the TOC slot at 24(sp),
so glfwd_resolve_one, which is not a leaf, overwrote the saved r5 and r6
on the first call through every slot. The first call of any GL entry point
taking a third or fourth integer argument was handed garbage. Reported
in issue 59 on the gtk3-demo ppc64le AppImage: the shipped gl-fwd.so exits
139 when preloaded, one built from this change stays alive. Not rerun here.
Registers are now saved above the 96-byte callee area (32-byte ELFv2
header plus a 64-byte parameter area). The frame is 288 bytes.
make gl-fwd-qemu-check gains a first-call test with eight live integer
arguments; it fails on the old frame (ppc64le) and passes on the new one.
The ELFv1 big-endian ppc64 trampoline block is removed. ppc64 is not
built (see docs/limits.md), nothing ran that block, and a ppc64 ELFv1
compile now stops at the existing "needs a tail-jump trampoline" error.
Requested by Samueru via errand.
Conversation: https://discord.com/channels/1313385177703256064/1562751109771300866
Transcript: https://talaria.qaidvoid.dev/?session=q19v2w4cx3o4