[PATCH] D58843: [WIP][MC][RISCV] Allow targets to defer forcing relocations

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

Hi @lewis-revill,

I've been looking into this and I have some observations, but not a satisfactory solution so far.

In the current form, this patch works fragment-wise. Which means that cross-fragment `pcrel_hi` / `pcrel_lo` can't really work because we eagerly solve `pcrel_hi` and later we can't relate `pcrel_lo` to its `pcrel_hi` because it is not in the current fragment. So we leave a dangling relocation for `pcrel_lo` which is problematic.

I moved the processing to be section-wise instead (i.e. delay all the handling until all the fragments of a section have been visited). This seems not to break anything but should be checked more thoroughly. The rest of the observations depend on this being possible.

Once we process section-wise what we see is that the offset between the `pcrel_lo` and the `pcrel_hi` is not correctly computed. This can be easily seen using `.p2align 4` instead of `2` so a bunch of `nop`s appear:

  $ cat t.s
  # pcrel_hi/pcrel_lo pair crossing a fragment boundary.
  
  .Lpcrel_label0:
  auipc  a0, %pcrel_hi(bar)
  
  .p2align  4   # Cause a new fragment be emitted here
  
  addi  a1, a0, %pcrel_lo(.Lpcrel_label0)
  
  bar:
    ret



  $ ./bin/llvm-mc -triple riscv64 -filetype obj -o t.o t.s && ./bin/llvm-objdump  -dr t.o
  
  t.o:	file format ELF64-riscv
  
  
  Disassembly of section .text:
  
  0000000000000000 .text:
         0: 17 05 00 00                  	auipc	a0, 0
         4: 13 00 00 00                  	nop
         8: 13 00 00 00                  	nop
         c: 13 00 00 00                  	nop
        10: 93 05 45 01                  	addi	a1, a0, 4
  
  0000000000000014 bar:
        14: 67 80 00 00                  	ret

The value computed in line 871, `FixedValue` has the wrong offset. The wrong offset is due to an interaction between `RISCVMCExpr::evaluatePCRelLo` computing a `MCValue` as stated in the comment ` (<real target> + <offset from this fixup to the auipc fixup>)` and  `MCExpr::evaluateAsRelocatable` removing the offset of the fixup within the section because this fixup is pc-relative. Thus, we compute 20 (the offset between the `addi` to `bar`, 4, and the offset from `addi` to `auipc`, 16) and then we remove 16 (the offset between the `addi` and `auipc`).

I'm unsure what we can do here, perhaps marking `pcrel_lo` fixups as **not** pc-relative would do? Maybe @jrtc27 or @asb have more insights whether this is feasible.

If we can't do this, then we need to make `RISCVMCExpr::evaluatePCRelLo` compensate the later subtraction (so it adds an extra `Layout->getFragmentOffset(Fragment)`). But to do this we need the `Fragment` of the current fixup (AFAIU it is not possible to get the `MCFragment` of a given `MCFixup`) and unfortunately this means we need to make it available to `MCExpr::evaluateAsRelocatable` (from `MCAssembler::evaluateFixupPreTarget`). This entails changing all the backends. I've gone this way and it seems to do the right thing for this case (i.e. the offset emitted is 20), but again needs some more testing.

I've also observed that in `RISCVMCExpr::evaluatePCRelLo`, the second check is tautological. Here `this` is `%pcrel_lo(.Lpcrel_label0)` and `AUIPCSRE` is `.Lpcrel_label0` so I fail to see how their fragments would ever be different.

  cpp
   // Don't try to evaluate %pcrel_hi/%pcrel_lo pairs that cross fragment
   // boundries.
   if (!AUIPCSRE ||
       findAssociatedFragment() != AUIPCSRE->findAssociatedFragment())

Hope these observations help!

Kind regards,


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58843/new/

https://reviews.llvm.org/D58843