[PATCH] D74198: [DebugInfo] Add support for DWARF64 into DWARFDebugAddr.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:159
+  if (Length) {
+    int LengthFieldWidth = (Format == dwarf::DwarfFormat::DWARF64) ? 16 : 8;
+    OS << format("Address table header: length = 0x%0*" PRIx64
----------------
ikudrin wrote:
> aprantl wrote:
> > unsigned?
> Could you explain why `unsinged` is better than `int` here?
> 
> Note that according to http://www.cplusplus.com/reference/cstdio/printf/,
> 
> > *: The width is not specified in the format string, but as an additional integer value argument preceding the argument that has to be formatted.
> 
> Not "an unsigned integer value".
I find the following link tends to be more precise and accurate in its wording:

https://en.cppreference.com/w/cpp/io/c/fprintf

> (optional) integer value or * that specifies minimum field width. The result is padded with space characters (by default), if required, on the left when right-justified, or on the right if left-justified. In the case when * is used, the width is specified by an additional argument of type `int`. If the value of the argument is negative, it results with the - flag specified and positive field width. (Note: This is the minimum width: The value is never truncated.)

I don't have a copy of the standard to see whether the type is explicitly mentioned, but that quote definitively says `int`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:57
+    // Check that we can read the extended unit length field.
+    if (!Data.isValidOffsetForDataOfSize(*OffsetPtr, 8)) {
+      invalidateLength();
----------------
IIRC, the `DataExtractor` class provides an optional second argument to its `get...` methods that can be used to specify an `Error` to pass any problems back in, including reading past the end. Would it make some sense to use that instead of the `isValid...` checks?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:71
     return createStringError(
-        errc::not_supported,
-        "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx64, Offset);
+        errc::invalid_argument,
+        "address table at offset 0x%" PRIx64
----------------
Maybe `errc::not_supported`?


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

https://reviews.llvm.org/D74198