[PATCH] D155357: [RISCV] Allow delayed decision for ADD/SUB relocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay created this revision.
MaskRay added reviewers: asb, compnerd, jrtc27, kito-cheng, reames.
Herald added subscribers: luke, VincentWu, vkmr, frasercrmck, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, shiva0217, niosHD, sabuasal, simoncook, johnrusso, rbar, hiraditya, arichardson, emaste.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added subscribers: llvm-commits, wangpc, eopXD.
Herald added a project: LLVM.

For a label difference `A-B` in assembly, if A and B are separated by a
linker-relaxable instruction, we should emit a pair of ADD/SUB
relocations (e.g. R_RISCV_ADD32/R_RISCV_SUB32,
R_RISCV_ADD64/R_RISCV_SUB64).

However, the decision is made upfront at parsing time with inadequate
heuristics (`requiresFixup`). As a result, LLVM integrated assembler
incorrectly suppresses R_RISCV_ADD32/R_RISCV_SUB32 for the following
code:

  // Simplified from a workaround https://android-review.googlesource.com/c/platform/art/+/2619609
  // Both end and begin are not defined yet. We decide ADD/SUB relocations upfront and don't know they will be needed.
  .4byte end-begin
  
  begin:
    call foo
  end:

To fix the bug, make two primary changes:

- Delete `requiresFixups` and the overridden emitValueImpl (from D103539 <https://reviews.llvm.org/D103539>). This deletion requires accurate evaluateAsAbolute (D153097 <https://reviews.llvm.org/D153097>).
- In MCAssembler::evaluateFixup, call handleAddSubRelocations to emit ADD/SUB relocations.

However, there is a remaining issue in
MCExpr.cpp:AttemptToFoldSymbolOffsetDifference. With MCAsmLayout, we may
incorrectly fold A-B even when A and B are separated by a
linker-relaxable instruction. This deficiency is acknowledged (see
D153097 <https://reviews.llvm.org/D153097>), but was previously bypassed by eagerly emitting ADD/SUB using
`requiresFixups`. To address this, we partially reintroduce `canFold` (from
D61584 <https://reviews.llvm.org/D61584>, removed by D103539 <https://reviews.llvm.org/D103539>).

Some expressions (e.g. .size and .fill) need to take the `MCAsmLayout`
code path in AttemptToFoldSymbolOffsetDifference, avoiding relocations
(weird, but matching GNU assembler and needed to match user
expectation). Switch to evaluateKnownAbsolute to leverage the `InSet`
condition.

As a bonus, this change allows for the removal of some relocations for
the FDE `address_range` field in the .eh_frame section.

riscv64-64b-pcrel.s contains the main test.
Add a linker relaxable instruction to dwarf-riscv-relocs.ll to test what
it intends to test.
Merge fixups-relax-diff.ll into fixups-diff.ll.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155357

Files:
  llvm/include/llvm/MC/MCAsmBackend.h
  llvm/lib/MC/MCAssembler.cpp
  llvm/lib/MC/MCExpr.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
  llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h
  llvm/test/CodeGen/RISCV/fixups-diff.ll
  llvm/test/CodeGen/RISCV/fixups-relax-diff.ll
  llvm/test/DebugInfo/RISCV/dwarf-riscv-relocs.ll
  llvm/test/DebugInfo/RISCV/relax-debug-frame.ll
  llvm/test/ExecutionEngine/JITLink/RISCV/anonymous_symbol.s
  llvm/test/MC/ELF/RISCV/gen-dwarf.s
  llvm/test/MC/RISCV/cfi-advance.s
  llvm/test/MC/RISCV/fde-reloc.s
  llvm/test/MC/RISCV/fixups-expr.s
  llvm/test/MC/RISCV/riscv64-64b-pcrel.s

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D155357.540645.patch
Type: text/x-patch
Size: 23030 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230715/6946ef35/attachment.bin>