[DebugInfo] Refine error messages in DWARFDebugAddr.

[ikudrin]

Summary

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

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

68

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]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

68

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]

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

68

That sounds good to me. Thanks!

[ikudrin]

• Update messages.

[ikudrin]

• Further update the messages changing ".debug_addr table..." to "address table..."

[jhenderson]

LGTM.

[dblaikie]

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

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

68

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

[ikudrin]

In D74196#1867749, @dblaikie wrote:

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?

[dblaikie]

In D74196#1868396, @ikudrin wrote:

In D74196#1867749, @dblaikie wrote:

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]

@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

68

See out-of-line comment.

[ikudrin]

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

[ikudrin]

• 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!

[jhenderson]

LGTM.

llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

if (UnitVersion >= 5) {
     HeaderData.Length = Data.getU32(OffsetPtr);
     if (HeaderData.Length == dwarf::DW_LENGTH_DWARF64) {
       invalidateLength();
       return createStringError(errc::not_supported,
           "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx64,
           HeaderOffset);
     }
     if (HeaderData.Length + sizeof(uint32_t) < sizeof(Header)) {
-      uint32_t TmpLength = getLength();
+      uint32_t TmpLength = HeaderData.Length;
       invalidateLength();
       return createStringError(errc::invalid_argument,
                          ".debug_addr table at offset 0x%" PRIx64
                          " has too small length (0x%" PRIx32
                          ") to contain a complete header",
                          HeaderOffset, TmpLength);
     }
     uint64_t End = HeaderOffset + getLength();
     if (!Data.isValidOffsetForDataOfSize(HeaderOffset, End - HeaderOffset)) {
-      uint32_t TmpLength = getLength();
+      uint32_t TmpLength = HeaderData.Length;
       invalidateLength();
       return createStringError(errc::invalid_argument,
           "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);
     }
 
     HeaderData.Version = Data.getU16(OffsetPtr);
     HeaderData.AddrSize = Data.getU8(OffsetPtr);
     HeaderData.SegSize = Data.getU8(OffsetPtr);
     DataSize = getDataSize();
   } else {
     HeaderData.Version = UnitVersion;
     HeaderData.AddrSize = AddrSize;
     // TODO: Support for non-zero SegSize.
     HeaderData.SegSize = 0;
     DataSize = Data.size();
   }
 
   // Perform basic validation of the remaining header fields.
 
   // We support DWARF version 5 for now as well as pre-DWARF5
   // implementations of .debug_addr table, which doesn't contain a header
   // and consists only of a series of addresses.
   if (HeaderData.Version > 5) {
-    return createStringError(errc::not_supported, "version %" PRIu16
-        " of .debug_addr section at offset 0x%" PRIx64 " is not supported",
-        HeaderData.Version, HeaderOffset);
+    return createStringError(errc::not_supported,
+                             ".debug_addr table at offset 0x%" PRIx64
+                             " has unsupported version %" PRIu16,
+                             HeaderOffset, HeaderData.Version);
   }
   // FIXME: For now we just treat version mismatch as an error,
   // however the correct way to associate a .debug_addr table
   // with a .debug_info table is to look at the DW_AT_addr_base
   // attribute in the info table.
   if (HeaderData.Version != UnitVersion)
     return createStringError(errc::invalid_argument,
                        ".debug_addr table at offset 0x%" PRIx64
                        " has version %" PRIu16
                        " which is different from the version suggested"
                        " by the DWARF unit header: %" PRIu16,
                        HeaderOffset, HeaderData.Version, UnitVersion);
   if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
     return createStringError(errc::not_supported,
                        ".debug_addr table at offset 0x%" PRIx64
-                       " has unsupported address size %" PRIu8,
+                       " has unsupported address size %" PRIu8
+                       " (4 and 8 supported)",
                        HeaderOffset, HeaderData.AddrSize);
   if (HeaderData.AddrSize != AddrSize && AddrSize != 0)
     WarnCallback(createStringError(
         errc::invalid_argument,
         ".debug_addr table at offset 0x%" PRIx64 " has address size %" PRIu8
         " which is different from CU address size %" PRIu8,
         HeaderOffset, HeaderData.AddrSize, AddrSize));
 

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_invalid_addr_size.s

 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
 # RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
 
 # CHECK: .debug_addr contents:
 # CHECK-NOT: {{.}}
-# ERR: unsupported address size 3
+# ERR: unsupported address size 3 (4 and 8 supported)
 # ERR-NOT: {{.}}
 
 # invalid addr size
   .section  .debug_addr,"",@progbits
 .Ldebug_addr0:
   .long 12 # unit_length = .short + .byte + .byte + .long + .long
   .short 5 # version
   .byte 3  # address_size
   .byte 0  # segment_selector_size
   .long 0x00000000
   .long 0x00000001

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_small_length_field.s

 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
 # RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
 
 # CHECK: .debug_addr contents:
 # CHECK-NOT: {{.}}
-# ERR: .debug_addr table at offset 0x0 has too small length (0x5) to contain a complete header
+# ERR: .debug_addr table at offset 0x0 has too small length (0x1) to contain a complete header
 # ERR-NOT: {{.}}
 
 # too small length value
   .section  .debug_addr,"",@progbits
 .Ldebug_addr0:
   .long 1  # unit_length
   .short 5 # version
   .byte 4  # address_size
   .byte 0  # segment_selector_size
   .long 0x00000000
   .long 0x00000001

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_section.s

 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
 # RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
 
 # CHECK: .debug_addr contents:
 # CHECK-NOT: {{.}}
-# ERR: section is not large enough to contain a .debug_addr table of length 0x10 at offset 0x0
+# ERR: section is not large enough to contain a .debug_addr table with length 0xc at offset 0x0
 # ERR-NOT: {{.}}
 
 # too small section to contain section of given length
   .section  .debug_addr,"",@progbits
 .Ldebug_addr0:
   .long 12 # unit_length
   .short 5 # version
   .byte 4  # address_size
   .byte 0  # segment_selector_size

llvm/test/tools/llvm-dwarfdump/X86/debug_addr_unsupported_version.s

 # RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
 # RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
 # RUN: FileCheck %s -input-file %t.err -check-prefix=ERR
 
-# ERR: version 6 of .debug_addr section at offset 0x0 is not supported
+# ERR: .debug_addr table at offset 0x0 has unsupported version 6
 # ERR-NOT: {{.}}
 
 # CHECK: .debug_addr contents
 # CHECK-NEXT:     length = 0x0000000c, version = 0x0005, addr_size = 0x04, seg_size = 0x00
 # CHECK-NEXT:     Addrs: [
 # CHECK-NEXT:     0x00000002
 # CHECK-NEXT:     0x00000003
 # CHECK-NEXT:     ]