[DWARFYAML][debug_info] Use 'AbbrCode' to index the abbreviation.

[Higuoxing]

Summary

Before this patch, we use (uint32_t)AbbrCode - (uint32_t)FirstAbbrCode to index the abbreviation. It's impossible for we to use the preceeding abbreviation of the previous one (e.g., if the previous DIE's AbbrCode is 2, we are unable to use the abbreviation with index 1). In this patch, we use AbbrCode to index the abbreviation directly.

[jhenderson]

llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml

632

Probably "ignores the contents of entries with abbrev code 0". (I wouldn't use the full term "abbreviation" as I think it's not particularly common usage in this context).

I'd also move this case later in the sequence (possibly the last one), since we should allow a non-zero value to be the "First" abbrev.

636

using the ... -> when the abbrev code is specified.

I think the value 2 is unimportant here.

642

using the ... -> when the abbrev code is specified to be lower than the first abbrev.

652–660

There's a bit of tension here between the AbbrOffset and the behaviour. Since the AbbrOffset should point to the start of the table containing the corresponding abbrevs, it's a little unclear what the behaviour should be if the offset is nonsensical. It also prevents us having multiple abbrev tables as it stands.

I think it's okay to add this test case for now, but I might suggest a change for the future, to split the abbrev section into distinct tables which can be referenced by some ID from the .debug_info. Different .debug_info tables can refer to the same or different abbrev tables that way. The offset would be auto-calculated to match, but could be overridden to a different value (the ID could still be used to actually identify how to write the values).

[grimar]

llvm/lib/ObjectYAML/DWARFVisitor.cpp

60

Doesn't seem this assert is helpful. AbbrevDecls is std::vector<Abbrev>,
Most (or all) of STL implementations check the out of bounds access internally I think.

Probably you should add a test case and do the following for now:

if (AbbrCode > DebugInfo.AbbrevDecls.size())
  report_fatal_error(...)

61

Since you are touching this line, could you convert the auto to an actual type?

[Higuoxing]

Address comments.

llvm/lib/ObjectYAML/DWARFVisitor.cpp

60

Sorry, I haven't tested a report_fatal_error() before. What will be checked? The dumped stack trace? or something else? Can I leave a "TODO" here and let this function return Error in a follow-up patch?

llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml

652–660

I think it's okay to add this test case for now, but I might suggest a change for the future, to split the abbrev section into distinct tables which can be referenced by some ID from the .debug_info. Different .debug_info tables can refer to the same or different abbrev tables that way. The offset would be auto-calculated to match, but could be overridden to a different value (the ID could still be used to actually identify how to write the values).

Yeah, the current implementation of the .debug_abbrev section doesn't support multiple tables. I deleted this test. I will it once generating multiple abbreviation tables is supported.

[jhenderson]

LGTM.

llvm/lib/ObjectYAML/DWARFVisitor.cpp

60

Typically, you wouldn't test a report_fatal_error as it shouldn't be possible to reach that bit of code. It's like an assert, but works without asserts being enabled too.

[grimar]

llvm/lib/ObjectYAML/DWARFVisitor.cpp

60

What will be checked? The dumped stack trace? or something else?

Just the fact of crash of the test case. In the follow-up you could fix the crash then.
It should be possible to check that crash with --crash option for not:

RUN: llvm-mc -filetype=obj -triple aarch64-windows %s -o - \
RUN: | not --crash llvm-readobj --unwind - 2>&1 | FileCheck %s

I do not mind leaving the test case for a follow-up. It was just important not to use an assert here.
report_fatal_error calls abort().

[Higuoxing]

Address comment.

llvm/lib/ObjectYAML/DWARFVisitor.cpp

60

Thanks! I added one more test case for it :-)

[Higuoxing]

Update.

[jhenderson]

LGTM, thanks.

llvm/lib/ObjectYAML/DWARFVisitor.cpp

60

I'd change "Report" to "Handle", since the error is now being reported (although not in the best way).

[jhenderson]

llvm/lib/ObjectYAML/DWARFVisitor.cpp

61

Should this message be "... less than or equal to ..."?

[Higuoxing]

Address review comments.

llvm/lib/ObjectYAML/DWARFVisitor.cpp

 //===--- DWARFVisitor.cpp ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //===----------------------------------------------------------------------===//
 
 #include "DWARFVisitor.h"
 #include "llvm/ObjectYAML/DWARFYAML.h"
+#include "llvm/Support/ErrorHandling.h"
 
 using namespace llvm;
 
 template <typename T>
 void DWARFYAML::VisitorImpl<T>::onVariableSizeValue(uint64_t U, unsigned Size) {
   switch (Size) {
   case 8:
     onValue((uint64_t)U);
static unsigned getRefSize(const DWARFYAML::Unit &Unit) {
   return getOffsetSize(Unit);
 }
 
 template <typename T> void DWARFYAML::VisitorImpl<T>::traverseDebugInfo() {
   for (auto &Unit : DebugInfo.CompileUnits) {
     onStartCompileUnit(Unit);
     if (Unit.Entries.empty())
       continue;
-    auto FirstAbbrevCode = Unit.Entries[0].AbbrCode;
 
     for (auto &Entry : Unit.Entries) {
       onStartDIE(Unit, Entry);
-      if (Entry.AbbrCode == 0u)
+      uint32_t AbbrCode = Entry.AbbrCode;
+      if (AbbrCode == 0 || Entry.Values.empty())
         continue;
-      auto &Abbrev = DebugInfo.AbbrevDecls[Entry.AbbrCode - FirstAbbrevCode];
+
+      if (AbbrCode > DebugInfo.AbbrevDecls.size())
+        // TODO: Report and test this error.
+        report_fatal_error("abbrev code must be less than the number of "
+                           "entries in abbreviation table");
+      const DWARFYAML::Abbrev &Abbrev = DebugInfo.AbbrevDecls[AbbrCode - 1];
       auto FormVal = Entry.Values.begin();
       auto AbbrForm = Abbrev.Attributes.begin();
       for (;
            FormVal != Entry.Values.end() && AbbrForm != Abbrev.Attributes.end();
            ++FormVal, ++AbbrForm) {
         onForm(*AbbrForm, *FormVal);
         dwarf::Form Form = AbbrForm->Form;
         bool Indirect;

llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml

 Sections:
   - Name: .debug_info
     Type: SHT_PROGBITS
     Size: 0x10
 
 ## d) Test that yaml2obj emits an error message when both the "Size" and the
 ## "debug_info" entry are specified at the same time.
 
-# RUN: not yaml2obj --docnum=4 %s 2>&1 | FileCheck %s --check-prefix=ERROR
+# RUN: not yaml2obj --docnum=4 %s 2>&1 | FileCheck %s --check-prefix=AMBIGUOUS-CONTENT
 
-# ERROR: yaml2obj: error: cannot specify section '.debug_info' contents in the 'DWARF' entry and the 'Content' or 'Size' in the 'Sections' entry at the same time
+# AMBIGUOUS-CONTENT: yaml2obj: error: cannot specify section '.debug_info' contents in the 'DWARF' entry and the 'Content' or 'Size' in the 'Sections' entry at the same time
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
- Length:
       UnitType:   DW_UT_type
       AbbrOffset: 0x1234
       AddrSize:   4
       Entries:    []
 
 ## e) Test that yaml2obj emits an error message when both the "Content" and the
 ## "debug_info" entry are specified at the same time.
 
-# RUN: not yaml2obj --docnum=5 %s 2>&1 | FileCheck %s --check-prefix=ERROR
+# RUN: not yaml2obj --docnum=5 %s 2>&1 | FileCheck %s --check-prefix=AMBIGUOUS-CONTENT
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
DWARF:
   debug_info:
     - Length:
         TotalLength: 0x1234
       Version:    5
       UnitType:   DW_UT_type
       AbbrOffset: 0x1234
       AddrSize:   4
       Entries:    []
+
+## h) Test that yaml2obj emits values in the DIE according to the abbreviation whose
+## index is equal to the value in 'AbbrevCode'.
+
+# RUN: yaml2obj --docnum=9 %s -o %t9.o
+# RUN: llvm-readelf --hex-dump=.debug_info %t9.o | \
+# RUN:   FileCheck %s --check-prefix=FORM --match-full-lines
+
+#       FORM: Hex dump of section '.debug_info':
+#  FORM-NEXT: 0x00000000 34120000 05000204 34120000 02341221 4.......4....4.!
+##                       ^-------                            unit_length (4-byte)
+##                                                  ^-       abbreviation code (ULEB128)
+##                                                    ^---   Form: DW_FORM_data2 (2-byte)
+##                                                        ^- Form: DW_FORM_data4 (4-byte)
+#  FORM-NEXT: 0x00000010 43658701 21436587 341200            Ce..!Ce.4..
+##                       ------
+##                             ^-                            abbreviation code (ULEB128)
+##                                ^-------                   Form: DW_FORM_data4 (4-byte)
+##                                         ^---              Form: DW_FORM_data2 (2-byte)
+##                                             ^-            abbreviation code (ULEB128)
+# FORM-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_abbrev:
+    - Tag:        DW_TAG_compile_unit
+      Children:   DW_CHILDREN_yes
+      Attributes:
+        - Attribute: DW_AT_low_pc
+          Form:      DW_FORM_data4
+        - Attribute: DW_AT_high_pc
+          Form:      DW_FORM_data2
+    - Tag:        DW_TAG_subprogram
+      Children:   DW_CHILDREN_no
+      Attributes:
+        - Attribute: DW_AT_low_pc
+          Form:      DW_FORM_data2
+        - Attribute: DW_AT_high_pc
+          Form:      DW_FORM_data4
+  debug_info:
+    - Length:
+        TotalLength: 0x1234
+      Version:    5
+      UnitType:   DW_UT_type
+      AbbrOffset: 0x1234
+      AddrSize:   4
+      Entries:
+        ## Test that yaml2obj emits values when the abbrev code is specified.
+        - AbbrCode: 2
+          Values:
+            - Value: 0x1234
+            - Value: 0x87654321
+        ## Test that yaml2obj emits values when the abbrev code is specified to
+        ## be lower than the first abbrev.
+        - AbbrCode: 1
+          Values:
+            - Value: 0x87654321
+            - Value: 0x1234
+        ## Test that yaml2obj ignores the contents of entries with abbrev code 0.
+        - AbbrCode: 0
+          Values:
+            - Value: 0x1234
+
+## i) Test that yaml2obj reports a fatal error when 'debug_info' has values in its
+## entries but 'debug_abbrev' doesn't have enough attributes for them.
+
+# RUN: not --crash yaml2obj --docnum=10 %s -o %t10.o 2>&1 | \
+# RUN:   tee %t10.log | FileCheck %s --check-prefixes=FATAL
+
+# FATAL: LLVM ERROR: abbrev code must be less than the number of entries in abbreviation table
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+DWARF:
+  debug_info:
+    - Length:
+        TotalLength: 0x1234
+      Version:    5
+      UnitType:   DW_UT_type
+      AbbrOffset: 0x1234
+      AddrSize:   4
+      Entries:
+        - AbbrCode: 1
+          Values:
+            - Value: 0x1234