[PATCH] D157802: [JITLink][EHFrameSupport] Accept multiple relocations

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

In D157802#4584034 <https://reviews.llvm.org/D157802#4584034>, @Hahnfeld wrote:

> In D157802#4583708 <https://reviews.llvm.org/D157802#4583708>, @lhames wrote:
>
>>> Multiple relocations at one offset are fine, for example RISC-V uses it in FDE's.
>>
>> That's interesting. Are all relocations that share an offset identical? (same type, target, etc.)
>
> No, they are different types (`ADD` and `SUB`) with different targets. I would have to look up the details, but you should be able to get an idea by looking at the test case in https://reviews.llvm.org/D157803. I *assume* that, because RISC-V doesn't have a specific delta relocation, CodeGen has to build it up by combining `ADD` and `SUB` relocations?

Funnily enough MachO doesn't have a delta either -- it has "paired relocations". So the `MachOLinkGraphBuilder` will look for pairs of `SUBTRACT` / `UNSIGNED` relocations and turn them into a Delta.

If paired ADDs and SUBs were the only combination that we had to support then I'd say that ELFLinkGraphBuilder should definitely do something similar, but it sounds like that's not the case.

>> If not, are we really free to ignore the others?
>
> I don't know. As far as I can tell, they are on a field of the FDE that the `EHFrameEdgeFixer` never looks at; but `BlockEdges` gathers all edges in the block, not only the ones that we need.
>
>> I'm particularly interested because I've thought about enforcing such a uniqueness constraint at the `LinkGraph` level: no more than one edge at a given offset. I had assumed that this would be safe, but it sounds like this may not work for RISCV graphs.
>
> No, this certainly will not work on RISC-V, also because of linker relaxation that works by having multiple relocations at the same offset.

I need to take a look at these. We shouldn't rule out the possibility of one-edge-per-offset: We might be able to represent the relaxations in other ways (e.g. new kinds, or a new metadata concept). We can hold off on enforcing any new constraints until we've had time to consider the design options though.

>> As for `BlockEdges`, it's dealing with a quirk that I think only applies to MachO: On some architectures, notably x86-64, the `__eh_frame` section doesn't have explicit relocations for some fields. Instead, the target is implied by the initial values of the CIE / FDE fields and the linker has to recreate the edges from that. BlockEdges I used to track which fields (if any) have explicit edges already so that the others can be created.
>
> Ah ok, thanks for that explanation. So `EHFrameEdgeFixer` is maybe not needed at all on ELF? Or maybe we can split the part that is specific to MachO into a separate pass that explicitly does what is needed instead of `getOrCreateEncodedPointerEdge`?

Yeah -- that sounds good to me. Could you see what happens if you just remove it altogether? Hopefully that just works. Otherwise we can look at refactoring options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157802