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

Mon Sep 18 15:10:05 PDT 2023

Hahnfeld added a comment.

In D157802#4647189 <https://reviews.llvm.org/D157802#4647189>, @jobnoorman wrote:

> In D157802#4645560 <https://reviews.llvm.org/D157802#4645560>, @lhames wrote:
>
>> For now we can kick the can down the road a bit by taking the `AddFDEToCIEEdges` approach. Did you get a chance to look at that?
>
> If I understand correctly, `AddFDEToCIEEdges` would implement a subset of what `EHFrameEdgeFixer` does, correct? If so, wouldn't it be easier (and duplicate less code) to ensure `EHFrameEdgeFixer` doesn't err on relocations it doesn't need anyway?

Yes, the idea was to move the necessary subset into a new pass `AddFDEToCIEEdges`. Code duplication is actually not that bad (IMHO), here is a prototype: https://github.com/hahnjo/llvm-project/tree/EHFrameSupport-AddFDEToCIEEdges it gets even a bit simpler if my analysis is correct and we can assume exactly one CFI record per block, cf. https://github.com/llvm/llvm-project/pull/66707.

Unfortunately, only adding the edges from FDE to CIE is not sufficient, we also need the keep-alive edges to the PC. So I would in principle agree on the strategy mentioned in https://reviews.llvm.org/D157803#inline-1546938 to pass `Edge::Invalid` and relax the requirement on multiple edges in that case, but the problem is that keeping the PC alive requires access to the edge at the PC-begin field, and for that I feel we should strongly assert that there is exactly one and not multiple... I don't have a good solution at the moment, but we may just have to bite the bullet and store a vector of edges at every offset, or just linearly search for the needed edge(s) - it's probably not that bad with only one CFI record per block?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157802