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

[Jan Kratochvil via Phabricator via llvm-commits]

jankratochvil marked 13 inline comments as done.
jankratochvil added inline comments.


================
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,
----------------
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`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104271