This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Refine error messages in DWARFDebugAddr.
ClosedPublic

Authored by ikudrin on Feb 6 2020, 10:45 PM.

Details

Summary

As a preparation for the subsequent patches, this updates the wordings of some error messages in DWARFDebugAddr.

Diff Detail

Event Timeline

ikudrin created this revision.Feb 6 2020, 10:45 PM
jhenderson added inline comments.Feb 7 2020, 1:01 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
65–66

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.

110

"4 and 8 are supported".

I might even go as far as "only sizes 4 and 8 are supported", but that might be a bit verbose for some people's tastes.

ikudrin marked 2 inline comments as done.Feb 7 2020, 1:58 AM
ikudrin added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
65–66

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.)

110

Thanks! I'll change that to "4 and 8 are supported" in the next update.

jhenderson added inline comments.Feb 7 2020, 3:16 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
65–66

That sounds good to me. Thanks!

ikudrin updated this revision to Diff 243366.Feb 8 2020, 6:26 AM
  • Update messages.
ikudrin updated this revision to Diff 243367.Feb 8 2020, 7:08 AM
  • Further update the messages changing ".debug_addr table..." to "address table..."
This revision is now accepted and ready to land.Feb 10 2020, 1:54 AM

Pleas commit the rename from ".debug_addr table" to "address table" separately.

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
65–66

@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)

Pleas commit the rename from ".debug_addr table" to "address table" separately.

Not sure I am following you. The purpose of this patch is to refine messages and change wordings here and there, so why this particular change should be separated?

Pleas commit the rename from ".debug_addr table" to "address table" separately.

Not sure I am following you. The purpose of this patch is to refine messages and change wordings here and there, so why this particular change should be separated?

Larger systemic changes like that are easier to review separately from more nuanced changes (this has more than just phrasing changes - and even if it did, I'd probably actually separate each phrasing change into its own commit - since they're not actually dependent on each other, but I'd make an exception for a mass renaming for consistency because it's easy enough to review as a batch (technically also easy to fail to spot that tests have appropriate updates when you're changing lots of diagnostics at the same time, but for such a mechanical change it's not so important to me that we check each one separately (wouldn't hurt, but I wouldn't insist on it)))

@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.

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp
65–66

See out-of-line comment.

So, let's make it "... to contain an address table at offset 0x... with a unit_length value of 0x...", right?

ikudrin updated this revision to Diff 243851.Feb 11 2020, 6:39 AM
  • Extracted the rename from ".debug_addr table" to "address table" into D74407
  • Updated a message about the insufficient size of the section according to the latest comments.
  • Changed a message about "a unit_length value which is too small to contain a complete header" similarly. Corrections are welcome!
This revision was automatically updated to reflect the committed changes.