[PATCH] D74196: [DebugInfo] Refine error messages in DWARFDebugAddr.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> @jhenderson - just curious, but could you provide an example/walkthrough (perhaps show specific examples, and the ways they could be interpreted correctly/incorrectly) of the ambiguity you were describing about the prior phrasing? I'm curious to better understand how phrasing these things might be problematic/improved.
> 
> (FWIW: I personally find "with a length field of" to be a bit awkward, but it is the most accurate - the other phrasing that comes to my mind is "with a specified length of <blah>" but that is then ambiguous ("how was the length specified?") in a way that the "with a length field of" is not ambiguous.
> 
> The "with a length field of" tweaks for me because it then starts to sound, to me, like the length field itself (imagine if it were say, encoded with variable length - so a large length value might itself be too large) might be causing things not to fit - rather than that the length describes a region too large for the section)

Imagine the section was one byte shorter than was required for the expected table, with the existing error message, you'd get something like "section is not large enough to contain a .debug_addr table of length 0x4000 at offset 0x0000". When you look at the section size in llvm-readelf, you would get a section size of "0x3fff", which is clearly less than the table length. On the other hand, the error message as @ikudrin originally proposed would be "section is not large enough to contain a .debug_addr table with length 0x3ffc at offset 0x0000", but 0x3ffc is smaller than 0x3fff, so if a user doesn't recognise that the length stated is the unit_length field, instead of the table's total length and doesn't include itself in the length, it may not be obvious what the problem is and could cause confusion. Maybe it should be "a unit_length value of" instead of a "with a length field of"? I think the term "value" here shows that it's a field in the format, rather than a higher-level concept.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:68
           "section is not large enough to contain a .debug_addr table "
-          "of length 0x%" PRIx32 " at offset 0x%" PRIx64,
+          "with length 0x%" PRIx32 " at offset 0x%" PRIx64,
           TmpLength, HeaderOffset);
----------------
dblaikie wrote:
> jhenderson wrote:
> > ikudrin wrote:
> > > jhenderson wrote:
> > > > I don't feel strongly about this, but I feel like the old message was a little clearer. If the error triggers with this change, I could see myself looking at the offset and section size and thinking, "hang on, that length should fit in that section", without realising that the length was specifically the length field.
> > > > 
> > > > If you want specifically to talk about the length value as recorded in the table, perhaps use "with a length field of ..."? If you do that, I'd bring the "at offset" bit before the "with a length" bit.
> > > What about "... to contain an address table at offset 0x... with a length field of 0x..."? Is it sounds more accurate? (Not being a native speaker, I will really appreciate any corrections of wordings.)
> > That sounds good to me. Thanks!
> @jhenderson - just curious, but could you provide an example/walkthrough (perhaps show specific examples, and the ways they could be interpreted correctly/incorrectly) of the ambiguity you were describing about the prior phrasing? I'm curious to better understand how phrasing these things might be problematic/improved.
> 
> (FWIW: I personally find "with a length field of" to be a bit awkward, but it is the most accurate - the other phrasing that comes to my mind is "with a specified length of <blah>" but that is then ambiguous ("how was the length specified?") in a way that the "with a length field of" is not ambiguous.
> 
> The "with a length field of" tweaks for me because it then starts to sound, to me, like the length field itself (imagine if it were say, encoded with variable length - so a large length value might itself be too large) might be causing things not to fit - rather than that the length describes a region too large for the section)
See out-of-line comment.


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

https://reviews.llvm.org/D74196