This is an archive of the discontinued LLVM Phabricator instance.

[WIP][MC][RISCV] Allow targets to defer forcing relocations
AbandonedPublic

Authored by lewis-revill on Mar 1 2019, 12:48 PM.

Details

Summary

This patch modifies the process of recording relocations and applying fixups for a fragment to allow the target to remain undecided about whether or not to force a relocation until all fixups have been evaluated.

Only after the target has been allowed to re-evaluate fixups that have been marked as unknown can the process of recording relocations and applying fixups be run.

For RISC-V, this allows us to determine exactly which %pcrel_hi and %pcrel_lo fixups need relocations by ensuring all %pcrel_lo fixups and corresponding %pcrel_hi fixups are correctly matched in this regard.

Notable changes for review:

  • Changed return type of shouldForceRelocation to Optional<bool> in MCAsmBackend
  • Added handleUnknownFixups to MCAsmBackend
  • Split up handleFixup and evaluateFixup to handleUnresolvedFixup, evaluateFixupPreTarget and evaluateFixup
  • Split up fixup handling code into two distinct passes, one to attempt to evaluate and resolve fixups, and another to report errors, record relocations, and apply fixups, with the target being allowed to intervene for unknown fixups in between the two passes

This patch is work in progress, I'd appreciate feedback on the general approach and disruptiveness of this change.

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Mar 1 2019, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 12:48 PM

I have yet to add a RISC-V test for the behaviour we're trying to fix, but for now see the discussion on D57240.

Remove dependency.

rogfer01 added inline comments.Apr 15 2019, 1:57 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
128

Would this hold if the resolution of https://github.com/riscv/riscv-elf-psabi-doc/issues/90 is that a %pcrel_lo can cross sections?

Perhaps I got this wrong, but I understand that this process is currently section-wise, so we can't say much about a %pcrel_hi not being referenced by any %pcrel_lo in the same section, can we?

Thanks. It looks like everyone is on board with mandating in the RISC-V ELF psABI specification that pcrel_lo and pcrel_hi stay in the same section. Since this is nominally an ABI break (just one that we think nobody will notice) I proposed giving it a one week countdown to officially merge -- essentially if nobody can figure out a use case for doing this within a week then we'll adopt the ABI change. More information is on the github: https://github.com/riscv/riscv-elf-psabi-doc/issues/90

Thanks for finding this!

lewis-revill added inline comments.Apr 23 2019, 7:30 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
128

Sorry for the delay, yes this is a completely section-wise process. If the resolution allowed the pcrel to cross sections this patch would not be sufficient and the fix would require storing references to fixups between sections, which couldn't really be done in a neat way.

bcain requested changes to this revision.Apr 23 2019, 8:01 AM
bcain added a subscriber: bcain.
bcain added inline comments.
include/llvm/MC/MCAsmBackend.h
84–85

Could you make it explicit in the doc comments here that a returned value of 'None' indicates that it's indeterminate/unknown whether relocation is required.

This revision now requires changes to proceed.Apr 23 2019, 8:01 AM

Rebased and added documentation comments

lewis-revill marked an inline comment as done.Apr 24 2019, 4:01 AM
bcain accepted this revision.Apr 24 2019, 7:12 AM
This revision is now accepted and ready to land.Apr 24 2019, 7:12 AM

Should the following case be considered for RISC-V? It's impossible to solve with this patch:

# pcrel_hi/pcrel_lo pair crossing a fragment boundary.

.Lpcrel_label0:
auipc  a0, %pcrel_hi(bar)

.p2align  2   # Cause a new fragment be emitted here

addi  a1, a0, %pcrel_lo(.Lpcrel_label0)

bar:
  # ...

I'm aware of the current clarification in the spec about crossing a section boundary, but this is less to do with specification and more about where LLVM decides to create fragments.

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 nops 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,

Hi @lewis-revill,

...

Hope these observations help!

Kind regards,

Hi @rogfer01,

Thanks for your thoughts, sorry for the delay.

I was worried that this would be the issue with this patch, in terms of not working section-wise. I would imagine that even if we did manage to make it work section-wise then that would not make a good generic patch... Thanks for the analysis on this approach though, I didn't realise that that would happen.

I'd like to know some more detail about the actual use cases for this pre-linker evaluation of fixups, it's becoming increasingly clear that a lot of difficulties could be/could have been avoided by letting the RISC-V shouldForceRelocation just return true for pcrel_hi/X_hi/pcrel_lo.

lewis-revill abandoned this revision.Aug 1 2019, 8:27 AM

Abandoned since this is no longer relevant to RISC-V; other individual fixes for issues relating to this problem have been merged since. I will keep D61907 around for discussion even though it's a similar 'big hammer' approach to the problem.