[DebugInfo]: Added support for DWARFv5 Info section header parsing in llvm-dwp utility.

[SouraVX]

Summary

This patch teaches llvm-dwp to parse DWARFv5 info section header. Tested this using asm test case caontaining DWARFv5 info. Assemling it to DWO object, checking corresponding content using llvm-dwarfdump. Then finally, packaging it to DWP using llvm-dwp and checking corresponding content using llvm-dwarfdump.

This patch is splitted from D74312, based on suggestions by @dblaikie

[dblaikie]

llvm/test/tools/llvm-dwp/X86/info-v5.s

25

Remove all the non-dwo sections (so the text section, and the debug sections that aren't dwo sections, etc).

69

Correct indentation. (elsewhere in this too - line up the assembly directives and the comments)

llvm/tools/llvm-dwp/llvm-dwp.cpp

156

Might be worth specifying the unit type that was found? At least even if it's just the integer value "unexpected unit type 0x3 found in debug_info".

This should also be tested. (the existing test case could have a second CU in it that just has an incorrect unit type in its header)

159–162

DWARFv5 reorders the abbreviation offset and the address size - so this code is /probably/ wrong/needs support for this part of DWARFv5. But that could wait for another patch (might be worth checking how the failure to parse that correctly is manifesting - if there's a good spot to demonstrate it in the test case by adding an extra CHECK line and a FIXME saying "hey, this is broken here/fix it soon" would be good)

[SouraVX]

Addressed comments by @dblaikie

[SouraVX]

llvm/tools/llvm-dwp/llvm-dwp.cpp

156

Wrapped this as string, since DWPError expects a string.

This should also be tested. (the existing test case could have a second CU in it that just has an incorrect unit type in its header)

I've tested it locally, error production works fine. Should I also add negative test case.?
If I add this in existing test case, then DWP packaging will detect/produce error and we'll loose primary verification for positive/Everthing fine case/ working llvm-dwp.

[dblaikie]

llvm/test/tools/llvm-dwp/X86/info-v5.s

25

Some indentation still needs fixing in this file - perhaps there's a tab/space issue, I'm not sure? But if you examine it here on the web you can see several are misaligned (DWARF version number comment, produce, language, name, dwo_name comments, external attribute, etc... )

llvm/tools/llvm-dwp/llvm-dwp.cpp

156

Yes, please add a test case. If adding it to the same file invalidates the existing test coverage there, then please add it as a separate test case.

159–162

DWARFv5 reorders the abbreviation offset and the address size - so this code is /probably/ wrong/needs support for this part of DWARFv5. But that could wait for another patch (might be worth checking how the failure to parse that correctly is manifesting - if there's a good spot to demonstrate it in the test case by adding an extra CHECK line and a FIXME saying "hey, this is broken here/fix it soon" would be good)

Any thoughts on this ^ ?

[SouraVX]

llvm/tools/llvm-dwp/llvm-dwp.cpp

159–162

Yes, I noticed this thing in Spec couple of times. But since llvm-dwarfdump is also not detecting while parsing, I took it as an expected/planned behavior.

[SouraVX]

llvm/test/tools/llvm-dwp/X86/info-v5.s

25

Strange, locally it's showing fine :)
Meanwhile, I'll add a negative test case, see if it works out second time.

[SouraVX]

Increased test coverage by adding negative test case.

[SouraVX]

Indentation is still a bit looking odd here, but it's fine on my machine and VI editor.

[dblaikie]

In D74425#1870680, @SouraVX wrote:

Indentation is still a bit looking odd here, but it's fine on my machine and VI editor.

Any chance there are tabs rather than spaces, or the wrong amount of tabs, or something? (I guess LLVM's assembly does have leading tabs and tabs between assembly directive and argument - but usually is pure spaces after that to the comment - so maybe you've got some tabs in unintended places)

[SouraVX]

In D74425#1870701, @dblaikie wrote:

In D74425#1870680, @SouraVX wrote:

Indentation is still a bit looking odd here, but it's fine on my machine and VI editor.

Any chance there are tabs rather than spaces, or the wrong amount of tabs, or something? (I guess LLVM's assembly does have leading tabs and tabs between assembly directive and argument - but usually is pure spaces after that to the comment - so maybe you've got some tabs in unintended places)

I'll look again closely. BTW, are you Okay with overall content of this patch ?

[SouraVX]

Indentation done, finally!

[dblaikie]

llvm/test/tools/llvm-dwp/X86/info-v5.s

5

Skip this - the DWARFv5 dwo files/dumping I think is already pretty well tested, I don't think there's much need to retest it here. (handy to test personally to validate some things, but probably not worth carrying as a checked in test)

10

Not sure what this check for debug_abbrev is? Check that it's present?

It might be more valuable to check the cu_index to check the contribution sizes of the various sections & demonstrate that the CU DWO ID was correctly parsed and inserted into the cu_index table. Please do that instead.

llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s

9 ↗

(On Diff #243985)

Rather than removing the unit type (& causing the address size to be read as the unit type) - please include an explicit unit type that is incorrect. I think that'll make the test easier to read - so it's obvious what value the error is referring to, rather than the comment misleadingly suggesting this is the address size field when it's really the unit type field.

llvm/tools/llvm-dwp/llvm-dwp.cpp

159–162

I don't think that's correct - it looks like this code is buggy but just not failing in a way that's been tested/examined yet.

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

The code should not be left as-is for very long because it is erroneously parsing the content & while the values may not currently be used (partly sub-optimal/possibly buggy, partly because they aren't needed) it's still a trap set to trip over anyone working with this code when that's not actually the order of fields in DWARFv5.

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.

[SouraVX]

Modified test cases.

[SouraVX]

llvm/tools/llvm-dwp/llvm-dwp.cpp

159–162

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

It;s getting used while skipping the DIE. Line:201. + I think, that has to be read anyways, to advance the offset/cursor by that amount for reading further data.

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.

Can you please, hint/describe a scenario, when AbbrevOffset different than zero in a DWO object.

[SouraVX]

Ping!

[dblaikie]

llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s

4 ↗

(On Diff #244016)

missing space between the unexpected unit type and the word 'found'.

(do other error messages use quotation marks around integer constants like this? I'd guess not & would recommend following that convention here - but could you check/verify that?)

llvm/tools/llvm-dwp/llvm-dwp.cpp

159–162

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

It;s getting used while skipping the DIE. Line:201. + I think, that has to be read anyways, to advance the offset/cursor by that amount for reading further data.

Then it seems important that this gets parsed correctly in v4 and v5 when the fields change location? Otherwise the address size that will be used would actually be the DWARF version number instead?

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.

Can you please, hint/describe a scenario, when AbbrevOffset different than zero in a DWO object.

The DWARF spec is generally fairly permissive - so "if it doesn't disallow it, it's probably/kind-of allowed". In this case there's no reason a DWARF producer couldn't hide an extra payload in debug_abbrev before the actual debug abbrev contribution, for instance.

At some point we might eventually want to figure out what/if it looks like to support LTO and Split DWARF which might involve multiple CUs in a single .dwo file, in which case then it could involve multiple debug_abbrev contributions and non-zero abbrev offsets.

[SouraVX]

nit: test case revised.

[SouraVX]

llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s

4 ↗

(On Diff #244016)

Their is as such precedent, although removed Single Quotes. One thing I noticed considering other error handling in DWP, We don't emit forms, simple fire an error

Line:170
eturn make_error<DWPError>("top level DIE is not a compile unit");

[dblaikie]

llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s

4 ↗

(On Diff #244016)

Yeah, I think that's an example of an error message that could be improved but hasn't been as yet.

Please remove the exclamation mark from the error message.

(hmm, it looks like the test has been updated but this test would currently fail because it looks like the implementation still has the same output format/bugs)

[fhahn]

It looks like the tests added in this patch are failing on macOS, because they require ELF object files. I've pushed rGc41a1f63b3c0 which adds a Linux triple to the tests. That should fix the failures on macOS

llvm/test/tools/llvm-dwp/X86/info-v5.s

+# this checks llvm-dwp handling of DWARFv5 Info section header.
+
+# RUN: llvm-mc --filetype=obj --split-dwarf-file=%t.dwo -dwarf-version=5 %s -o %t.o
+# RUN: llvm-dwarfdump -v %t.dwo | FileCheck %s
+
+# RUN: llvm-dwp %t.dwo -o %t.dwp
+# RUN: llvm-dwarfdump -v %t.dwp | FileCheck %s
+
+#CHECK-DAG: .debug_abbrev.dwo contents:
+#CHECK-DAG: .debug_info.dwo contents:
+#CHECK: 0x00000000: Compile Unit: length = 0x00000050 version = 0x0005 unit_type = DW_UT_split_compile abbr_offset = 0x0000 addr_size = 0x08 DWO_id = 0xefb76c21dffbf806 (next unit at 0x00000054)
+
+        .text
+  .file "int.c"
+  .file 0 "/dwp" "int.c" md5 0x2db7aefc947798db77723c8eecad9077
+  .type integer,@object         # @integer
+  .data
+  .globl  integer
+  .p2align  2
+integer:
+.Linteger$local:
+  .long 4                       # 0x4
+  .size integer, 4
+
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                       # Abbreviation Code
+  .byte 74                      # DW_TAG_skeleton_unit
+  .byte 0                       # DW_CHILDREN_no
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte 27                      # DW_AT_comp_dir
+  .byte 8                       # DW_FORM_string
+  .ascii  "\264B"                 # DW_AT_GNU_pubnames
+  .byte 25                      # DW_FORM_flag_present
+  .byte 118                     # DW_AT_dwo_name
+  .byte 8                       # DW_FORM_string
+  .byte 115                     # DW_AT_addr_base
+  .byte 23                      # DW_FORM_sec_offset
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+  .section  .debug_info,"",@progbits
+.Lcu_begin0:
+  .long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+  .short  5                       # DWARF version number
+  .byte 4                       # DWARF Unit Type
+  .byte 8                       # Address Size (in bytes)
+  .long .debug_abbrev           # Offset Into Abbrev. Section
+  .quad -1173350285159172090
+  .byte 1                       # Abbrev [1] 0x14:0xf DW_TAG_skeleton_unit
+  .long .Lline_table_start0     # DW_AT_stmt_list
+  .asciz  "/home/sourabh/work/dwarf/c_c++/dwp"                       # DW_AT_comp_dir
+                                        # DW_AT_GNU_pubnames
+  .asciz  "int.dwo"                       # DW_AT_dwo_name
+  .long .Laddr_table_base0      # DW_AT_addr_base
+.Ldebug_info_end0:
+  .section  .debug_info.dwo,"e",@progbits
+  .long .Ldebug_info_dwo_end0-.Ldebug_info_dwo_start0 # Length of Unit
+.Ldebug_info_dwo_start0:
+  .short  5                       # DWARF version number
+  .byte 5                       # DWARF Unit Type
+  .byte 8                       # Address Size (in bytes)
+  .long 0                       # Offset Into Abbrev. Section
+  .quad -1173350285159172090
+  .byte 1                       # Abbrev [1] 0x14:0x16 DW_TAG_compile_unit
+  .asciz  "clang version 11.0.0"  # DW_AT_producer
+  .short  12                      # DW_AT_language
+        .asciz  "int.c"               # DW_AT_name
+  .asciz  "int.dwo"               # DW_AT_dwo_name
+  .byte 2                       # Abbrev [2] 0x1a:0xb DW_TAG_variable
+  .asciz  "integer"                       # DW_AT_name
+  .long 37                      # DW_AT_type
+                                        # DW_AT_external
+  .byte 0                       # DW_AT_decl_file
+  .byte 1                       # DW_AT_decl_line
+  .byte 2                       # DW_AT_location
+  .byte 161
+  .byte 0
+  .byte 3                       # Abbrev [3] 0x25:0x4 DW_TAG_base_type
+  .asciz  "int"                   # DW_AT_name
+  .byte 5                       # DW_AT_encoding
+  .byte 4                       # DW_AT_byte_size
+  .byte 0                       # End Of Children Mark
+.Ldebug_info_dwo_end0:
+  .section  .debug_abbrev.dwo,"e",@progbits
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 37                      # DW_AT_producer
+  .byte 8                       # DW_FORM_string
+  .byte 19                      # DW_AT_language
+  .byte 5                       # DW_FORM_data2
+  .byte 3                       # DW_AT_name
+  .byte 8                       # DW_FORM_string
+  .byte 118                     # DW_AT_dwo_name
+  .byte 8                       # DW_FORM_string
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 2                       # Abbreviation Code
+  .byte 52                      # DW_TAG_variable
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 8                       # DW_FORM_string
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 63                      # DW_AT_external
+  .byte 25                      # DW_FORM_flag_present
+  .byte 58                      # DW_AT_decl_file
+  .byte 11                      # DW_FORM_data1
+  .byte 59                      # DW_AT_decl_line
+  .byte 11                      # DW_FORM_data1
+  .byte 2                       # DW_AT_location
+  .byte 24                      # DW_FORM_exprloc
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 3                       # Abbreviation Code
+  .byte 36                      # DW_TAG_base_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 8                       # DW_FORM_string
+  .byte 62                      # DW_AT_encoding
+  .byte 11                      # DW_FORM_data1
+  .byte 11                      # DW_AT_byte_size
+  .byte 11                      # DW_FORM_data1
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+  .section  .debug_addr,"",@progbits
+  .long .Ldebug_addr_end0-.Ldebug_addr_start0 # Length of contribution
+.Ldebug_addr_start0:
+  .short  5                       # DWARF version number
+  .byte 8                       # Address size
+  .byte 0                       # Segment selector size
+.Laddr_table_base0:
+  .quad integer
+.Ldebug_addr_end0:
+  .section  .debug_gnu_pubnames,"",@progbits
+  .long .LpubNames_end0-.LpubNames_begin0 # Length of Public Names Info
+.LpubNames_begin0:
+  .short  2                       # DWARF Version
+  .long .Lcu_begin0             # Offset of Compilation Unit Info
+  .long 35                      # Compilation Unit Length
+  .long 26                      # DIE offset
+  .byte 32                      # Attributes: VARIABLE, EXTERNAL
+  .asciz  "integer"               # External Name
+  .long 0                       # End Mark
+.LpubNames_end0:
+  .section  .debug_gnu_pubtypes,"",@progbits
+  .long .LpubTypes_end0-.LpubTypes_begin0 # Length of Public Types Info
+.LpubTypes_begin0:
+  .short  2                       # DWARF Version
+  .long .Lcu_begin0             # Offset of Compilation Unit Info
+  .long 35                      # Compilation Unit Length
+  .long 37                      # DIE offset
+  .byte 144                     # Attributes: TYPE, STATIC
+  .asciz  "int"                   # External Name
+  .long 0                       # End Mark
+.LpubTypes_end0:
+  .ident  "clang version 11.0.0"
+  .section  ".note.GNU-stack","",@progbits
+  .addrsig
+  .section  .debug_line,"",@progbits
+.Lline_table_start0:

llvm/tools/llvm-dwp/llvm-dwp.cpp

 static Expected<CompileUnitIdentifiers> getCUIdentifiers(StringRef Abbrev,
                                                          StringRef Info,
                                                          StringRef StrOffsets,
                                                          StringRef Str) {
   uint64_t Offset = 0;
   DataExtractor InfoData(Info, true, 0);
   dwarf::DwarfFormat Format = dwarf::DwarfFormat::DWARF32;
   uint64_t Length = InfoData.getU32(&Offset);
+  CompileUnitIdentifiers ID;
+  Optional<uint64_t> Signature = None;
   // If the length is 0xffffffff, then this indictes that this is a DWARF 64
   // stream and the length is actually encoded into a 64 bit value that follows.
   if (Length == 0xffffffffU) {
     Format = dwarf::DwarfFormat::DWARF64;
     Length = InfoData.getU64(&Offset);
   }
   uint16_t Version = InfoData.getU16(&Offset);
+  if (Version >= 5) {
+    auto UnitType = InfoData.getU8(&Offset);
+    if (UnitType != dwarf::DW_UT_split_compile)
+      return make_error<DWPError>(
+          "unit type DW_UT_split_compile type not found in debug_info header");
+  }
   InfoData.getU32(&Offset); // Abbrev offset (should be zero)
   uint8_t AddrSize = InfoData.getU8(&Offset);
-
+  if (Version >= 5)
+    Signature = InfoData.getU64(&Offset);
   uint32_t AbbrCode = InfoData.getULEB128(&Offset);
 
   DataExtractor AbbrevData(Abbrev, true, 0);
   uint64_t AbbrevOffset = getCUAbbrev(Abbrev, AbbrCode);
   auto Tag = static_cast<dwarf::Tag>(AbbrevData.getULEB128(&AbbrevOffset));
   if (Tag != dwarf::DW_TAG_compile_unit)
     return make_error<DWPError>("top level DIE is not a compile unit");
   // DW_CHILDREN
   AbbrevData.getU8(&AbbrevOffset);
   uint32_t Name;
   dwarf::Form Form;
-  CompileUnitIdentifiers ID;
-  Optional<uint64_t> Signature = None;
   while ((Name = AbbrevData.getULEB128(&AbbrevOffset)) |
          (Form = static_cast<dwarf::Form>(AbbrevData.getULEB128(&AbbrevOffset))) &&
          (Name != 0 || Form != 0)) {
     switch (Name) {
     case dwarf::DW_AT_name: {
       Expected<const char *> EName =
           getIndexedString(Form, InfoData, Offset, StrOffsets, Str);
       if (!EName)
         return EName.takeError();
       ID.Name = *EName;
       break;
     }
-    case dwarf::DW_AT_GNU_dwo_name: {
+    case dwarf::DW_AT_GNU_dwo_name:
+    case dwarf::DW_AT_dwo_name: {
       Expected<const char *> EName =
           getIndexedString(Form, InfoData, Offset, StrOffsets, Str);
       if (!EName)
         return EName.takeError();
       ID.DWOName = *EName;
       break;
     }
     case dwarf::DW_AT_GNU_dwo_id: