[JITLink] Some cleanups to EHFrameSupport

[hahnjo]

• Remove unused variable.
• Error on existing edge at CIE pointer field.
• Simplify CFI processing in EHFrameEdgeFixer: The code expects DWARFRecordSectionSplitter to split each CFI record into its own block, so remove loop over possibly multiple entries in one block.

[hahnjo]

ping @lhames @mtvec

[lhames]

Why error on an existing CIE edge?

LGTM otherwise.

[hahnjo]

Why error on an existing CIE edge?

Because that's the simplification, that the new code expects that there is no edge yet and we can remove the additional checks. And I firmly believe that assumptions should be checked in the code.

[lhames]

There were multiple simplifications in this piece of code. Assuming one record per block seems reasonable (we have the splitter as you noted), but rejecting objects with existing FDE-to-CIE edges is a change in behavior -- I'm asking for the reasoning behind that.

[lhames]

Making my concern explicit: This could cause us to reject well-formed objects that would have been accepted before. The existing error check for that doesn't seem burdensome in either readability or performance -- I think that should have been left it.

Sorry -- probably should have been clearer about that in the review.

[hahnjo]

This could cause us to reject well-formed objects that would have been accepted before.

According to the DWARF standard:

CIE_pointer
A constant offset into the .debug_frame section that denotes the CIE that is associated with this FDE.

So the field in the emitted objects must always be offsets while only dynamically registered frames must have a finalized pointer, which we achieve by adding edges / relocations. In my reading this means we must never see a relocation on this field before we process it...

[lhames]

The DWARF spec is handy for understanding eh-frames for exceptions but I don't think it constitutes a proper spec for them. E.g. the CIE_pointer in eh-frames is actually a delta back to the CIE, rather than being an offset within the section as the docs claim. (And I think the idea of this being a link-time constant seems incompatible with ld -r in general).

The practical problem is just Darwin as far as I know: On some architectures (e..g arm64) we use relocations for some fields (pc-begin consistently, but others too). LLVM doesn't generate CIE edges (at least not these days), but I have seen them in the past, possibly from custom compilers. I'm inclined to pay the extra cost to check for and accept them where they're present.