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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin marked 2 inline comments as done.
ikudrin added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:226
 public:
   // Each FDE has a CIE it's "linked to". Our FDE contains is constructed with
   // an offset to the CIE (provided by parsing the FDE header). The CIE itself
----------------
jhenderson wrote:
> 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.
The comment is about the laziness of acquiring the CIE, but it looks like that was never true, even from the [[ https://reviews.llvm.org/rGfd08bc195b36b2262f9ee4b283eef44b0520ebed | beginning ]]. I guess it is better to remove this comment altogether.


================
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=";
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74613