gas, bfd: keep the target offset in deferred hunk pc-relative relocs - #21
Conversation
gas cancelled the whole fx_offset for a pc-relative fixup deferred to the linker, so a bra.l/bsr.l to a symbol that gas had already turned into section+offset landed at the start of the section (gcc sibcalls to a function defined earlier in the same file). Keep the offset in the field, subtract only the symbol value the hunk writer re-adds for weak symbols, and drop the linker workaround that assumed a non-zero field was the weak encoding carrying the PC bias.
There was a problem hiding this comment.
🟡 Changes recommended
Same-section weak branches may still be incorrectly biased.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes Amiga Hunk PC-relative relocations that lost target section offsets.
Changes:
- Preserves target offsets in deferred GAS relocations.
- Removes obsolete weak-relocation compensation in BFD.
File summaries
| File | Review |
|---|---|
gas/config/tc-m68k.c |
Updates deferred relocation handling. Moderate: same-section weak branches may remain doubly biased; forced symbols must be treated as deferred. Nit: add Hunk regression coverage for strong, undefined, weak, and explicit-addend cases. |
bfd/amigaoslink.c |
Removes the legacy weak-target displacement workaround. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (sym->section != current_section | ||
| && strcmp (sym->section->name, current_section->name)) | ||
| return fixP->fx_offset; | ||
| return 0; |
There was a problem hiding this comment.
Right, a weak symbol defined in the current section stayed on the local path and got the PC subtracted twice. Now deferred whenever S_FORCE_RELOC is true for the symbol; the new ld-amiga/pcrel test covers that case (wloc).
| if (sym->section != current_section | ||
| && strcmp (sym->section->name, current_section->name)) | ||
| return fixP->fx_offset; | ||
| return 0; |
There was a problem hiding this comment.
Added gas/testsuite/gas/m68k/amiga-pcrel.{s,d} for the reloc fields (strong and weak section-folded targets, explicit addends, undefined symbols, weak in the same section) and a new ld/testsuite/ld-amiga/pcrel.d that links two objects and checks every bra.l lands on its symbol. Both fail on the previous code and pass now.
A weak symbol is kept in the reloc even when defined in the current section, so the linker resolves the branch and gas must not subtract the PC base itself. Spotted by Copilot on #21.
amiga-pcrel checks the field gas leaves for deferred pc-relative relocs to a strong, weak, offset and undefined target; ld-amiga/pcrel links two objects and checks every bra.l lands on its symbol.
|
@copilot PTAL |
Reviewed the latest commits and follow-up fixes; I don’t see any remaining issues to address in this PR from my side. |
A
bra.l/bsr.lto a symbol that gas has already turned into section+offset was linked to the start of the section instead of the symbol. gcc emits these for every sibcall (jra), so any function that tail-calls another function defined earlier in the same file jumped to the wrong place. In the AmigaOS-hosted cc1,global_range_query::~global_range_querytail-branched intorange_query::range_on_edgeinstead ofrange_query::~range_query, 0x788 bytes further, and cc1 crashed at exit after writing correct output.Three-line reproducer:
Built with
-Os -mcpu=68040 -ffunction-sections,fends withbraltopad, the first function of.text, instead offar.Cause: for a pc-relative fixup deferred to the linker,
md_pcrel_from_m68kreturnedfx_offset, so the generic code subtracted the whole offset and stored a zero field. That was meant to give the linker a zero addend, but when the symbol has been replaced by its section the offset of the symbol inside it lives infx_offsettoo, and the hunk reloc has nowhere else to carry it.Fix:
+= reloc->addressin that path was the origin of the extra bias.Verified with an assembly matrix (strong, weak, external, with and without addend, absolute and pc-relative) and two C reproducers including a weak COMDAT sibcall, all landing on the right symbol. The gas m68k testsuite gives identical results before and after (8 passes, the same 27 pre-existing ColdFire and ELF-format failures).
Every m68k object assembled by the current 16.2 toolchains can carry mis-linked branches, so the target libraries and the hosted packages need a rebuild with this gas.