This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix printing CIE offsets in EH FDEs.
ClosedPublic

Authored by ikudrin on Feb 14 2020, 7:06 AM.

Details

Summary

While in .debug_frame a value of the CIE pointer field is an offset to a corresponding CIE record from the beginning of the section, in .eh_frame it is relative to the current offset. Previously, when we dumped FDE records, we just printed the same value for both the CIE pointer field and the CIE offset, which worked for .debug_frame sections but was wrong for .eh_frame ones. This patch fixes the issue by printing the kept offset of the linked CIE object.

Diff Detail

Event Timeline

ikudrin created this revision.Feb 14 2020, 7:06 AM

Just making sure I understand: in .debug_frame, the cie_pointer field is always an absolute offset, whereas in .eh_frame, it is a relative value from the field, but we weren't making this distinction before, right? What does e.g. classic dwarfdump or GNU readelf display?

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
225–226

Could you fix the second sentence of this comment (possibly as a separate commit). I think the "contains" needs deleting.

It might also be worth saying whether the offset is relative or from the start of the section.

248–249

It may be worth making a note of the eh_frame/debug_frame difference here.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
317

I'm confused by the casting to 32-bits. I take it this code isn't DWARF64 ready yet?

Also, I'm not sure CIEPointer should be an unsigned type for .eh_frame at least. In the Ian Lance Taylor blog on the topic (https://www.airs.com/blog/archives/460), which I use as my .eh_frame "standard", it talks about how this value can be negative or positive. Do we have examples in tests which show the behaviour for positive and negative values?

ikudrin marked 2 inline comments as done.Feb 18 2020, 4:26 AM
ikudrin added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
225–226

The comment is about the laziness of acquiring the CIE, but it looks like that was never true, even from the beginning. I guess it is better to remove this comment altogether.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
317

I'm confused by the casting to 32-bits. I take it this code isn't DWARF64 ready yet?

Right, not supporting 64-bit values yet. The truncation is fixed in D73887, which is the final patch of this set.

Also, I'm not sure CIEPointer should be an unsigned type for .eh_frame at least. In the Ian Lance Taylor blog on the topic (https://www.airs.com/blog/archives/460), which I use as my .eh_frame "standard", it talks about how this value can be negative or positive.

He does not talk about negative values, only that "A positive value goes backward; that is, you have to subtract the value of the ID field from the current byte position to get the CIE position." Also, note that in https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html the value of the CIE pointer is explicitly described as an unsigned value.

ikudrin updated this revision to Diff 245140.Feb 18 2020, 6:01 AM
ikudrin retitled this revision from [DebugInfo] Fix printing CIE offset in EH FDEs. to [DebugInfo] Fix printing CIE offsets in EH FDEs..
  • Added a comment for CIEPointer describing the difference in EH and debug FDEs.
ikudrin marked an inline comment as done.Feb 18 2020, 6:04 AM
ikudrin added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
225–226

I've added D74761 to remove the comment. What do you think?

jhenderson accepted this revision.Feb 24 2020, 3:53 AM

LGTM, with one nit.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
250

Strictly speaking, the DWARF standard doesn't define the eh_frame format, so I think I'd replace "debug" here with "DWARF" to emphasise that difference.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
317

Yeah, my bad. I misread.

This revision is now accepted and ready to land.Feb 24 2020, 3:53 AM
This revision was automatically updated to reflect the committed changes.