Page MenuHomePhabricator

[RISCV] Implement getExprForFDESymbol to ensure RISCV_32_PCREL is used for the FDE location

Authored by asb on Aug 19 2019, 8:04 AM.



Follow binutils in using RISCV_32_PCREL for the FDE initial location. As explained in the relevant binutils commit , the ADD/SUB pair of relocations is problematic in the presence of linker relaxation.

This patch has the same end goal as D64715 but includes test changes and avoids adding a new global VariantKind to MCExpr.h (preferring RISCVMCExpr VKs like the rest of the RISC-V backend).

Diff Detail


Event Timeline

asb created this revision.Aug 19 2019, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2019, 8:04 AM
asb updated this revision to Diff 215905.Aug 19 2019, 8:09 AM
luismarques added inline comments.Aug 20 2019, 4:41 AM
41 ↗(On Diff #215905)

Typo in "location".

45 ↗(On Diff #215905)

LLVM has dropped support for DWARF64, so this should be correct more or less indefinitely, but this code is implicitly assuming that Encoding includes dwarf::DW_EH_PE_sdata4. Maybe assert that's the case, to trip if that ever changes?

12 ↗(On Diff #215905)

The issue doesn't seem to be specific to this patch, but when you insert instructions before .cfi_startproc the relocation doesn't seem to be updated correctly, unlike with binutils. Might be worth looking where EmitPersonality gets its Symbol from.

luismarques accepted this revision.Aug 20 2019, 4:44 AM

Overall LGTM. Can be merged after fixing at least the typo.

This revision is now accepted and ready to land.Aug 20 2019, 4:44 AM
asb marked 3 inline comments as done.Aug 20 2019, 5:28 AM
asb added inline comments.
12 ↗(On Diff #215905)

I had a look and actually couldn't reproduce this issue when using GNU readelf -r on both llvm-mc and GNU as output.

This revision was automatically updated to reflect the committed changes.