[PATCH] D104271: llvm-dwarfdump: Print warnings on invalid DWARF

[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 possible suggestion, but also happy if this is committed as-is. You might want to wait for @dblaikie too.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:372-375
+  static const char *getSupportedAddressSizes() { return "2, 4, 8"; }
   static bool isAddressSizeSupported(unsigned AddressSize) {
     return AddressSize == 2 || AddressSize == 4 || AddressSize == 8;
   }
----------------
This is probably absolutely fine, but I was thinking about it and wondering whether it would make some sense to factor out the commonality into some sort of container, that the string function can iterate over to generate a string, and the bool function can just compare values against. What do you think?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:37
+        createStringError(errc::invalid_argument,
+                          "DWARF unit [0x%8.8" PRIx64 ", 0x%8.8" PRIx64 ") "
+                          "tries to read DIEs at offset 0x%8.8" PRIx64,
----------------
jankratochvil wrote:
> jhenderson wrote:
> > jankratochvil wrote:
> > > jhenderson wrote:
> > > > It's not going to be clear to the end user that these two values represent offsets. I'd be more explicit: "DWARF unit from offset x to offset y ..."
> > > > 
> > > > Same applies below.
> > > I haven't changed this yet. I disagree with "from offset x to offset y" as that would need to be rather "from offset x incl. to offset y excl." which already looks to me too talkative. Primarily as this message is for DWARF developers, not for end users.
> > > And then we should change an already existing error message: [[ https://github.com/llvm/llvm-project/blob/ffa252e8ce2465893f2c798b0586695f03a68d18/llvm/lib/Support/DataExtractor.cpp#L26 | "while reading [0x%x, 0x%x)" ]]
> > > 
> > With at least offsets, I would never read "from offset X to offset Y" as inclusive at both ends, because of the nature of what an offset represents. But maybe it is a real issue. You could achieve the same meaning, without the ambiguity risk by saying "with length X at offset Y" instead, for example. The length is actually the thing that's encoded in the DWARF after all. You could make it slightly less talkative like this: "DWARF unit (offset 0x1234, length 0x4321) tries to ...".
> > 
> > I don't think the two error messages are quite equivalent. In the DataExtractor one, the message talks about reading the range, and therefore it's somewhat clearer that you're dealing with an offset, whereas here the numbers in the range aren't things that are mentioned as being read or similar, so you lose that context.
> > without the ambiguity risk by saying "with length X at offset Y" instead
> 
> That is definitely ambiguous as it can mean length `b-a` of `[a, b)` or it can mean length stored in the binary which is `b-a-4` (for DWARF32). I used the offsets with "incl." and "excl." to move forward.
> 
> > The length is actually the thing that's encoded in the DWARF after all.
> 
> So you did mean the `b-a-4`.
> 
Fair point. Let's stick with your latest version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104271