[PATCH] D74613: [DebugInfo] Fix printing CIE offsets in EH FDEs.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with one nit.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:250
+  /// The following fields are defined in section 6.4.1 of the DWARFv3 standard.
+  /// Note that CIE pointers in EH FDEs, unlike debug FDEs, contain relative
+  /// offsets to the linked CIEs. See the following link for more info:
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:317
+  OS << format("%08x %08x %08x", (uint32_t)Offset, (uint32_t)Length,
+               (uint32_t)CIEPointer)
+     << " FDE cie=";
----------------
ikudrin wrote:
> jhenderson wrote:
> > 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?
> > 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.
Yeah, my bad. I misread.


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

https://reviews.llvm.org/D74613