DWARF: Fix a bug in array size computation

[labath]

Summary

r346165 introduced a bug, where we would fail to parse the size of an
array if that size happened to match an existing die offset.

The logic was:
if (DWARFDIE count = die.GetReferencedDie(DW_AT_count))

num_elements = compute_vla_size(count);

else

num_elements = die.GetUsigned(DW_AT_count); // a fixed-size array

The problem with this logic was that GetReferencedDie did not take the
form class of the attribute into account, and would happily return a die
reference for any form, if its value happened to match some die.

As this behavior is inconsistent with how llvm's DWARFFormValue class
operates, I chose to fix the problem by making our version of this class
match the llvm behavior. For this to work, I had to add an explicit form
class check to the .apple_XXX tables parsing code, because they do
(incorrectly?) use data forms as die references.

[clayborg]

source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

619 ↗

(On Diff #179254)

DW_FORM_ref_sig8 is a type signature and shouldn't be returned as a valid DWARF DIE reference. In the .debug_types patch, this uses m_cu to get to the debug info and uses the type signature to DWARF offset map to return a valid .debug_info offset. If the DWARF is older where we have both .debug_info and .debug_types. we add the size of the .debug_info to the result since we concatenate the .debug_info and .debug_types and treat it as one large DWARF section

620 ↗

(On Diff #179254)

Not sure that this is, but if it isn't an absolute .debug_info offset, then we shouldn't return it here

639 ↗

(On Diff #179254)

Ditto above

641 ↗

(On Diff #179254)

Ditto above

[labath]

Hm.. I can't say I have given much thought about this beforehand, as I just copied this code from llvm. However, now that I did think about it, I believe the current implementation makes sense. Nothing about DWARFFormValue::Reference (well, except the local variable name, which I guess I should rename) says it must return a DIE offset. DW_FORM_ref_sig8 is not a die offset, but it is certainly a DIE reference, albeit one that must be resolved in a more complicated way. This is also the view taken by the DWARF 5 spec (section 7.5.5 "Classes and Forms" lists DW_FORM_ref_sig8 as a reference form). And I expect this is the what the llvm implementation is based on.

So, in order to minimize confusion, I think we should stick to this definition. The code for resolving the more complex reference forms can live in some higher level code (DWARFDIE::GetReferencedDie ?), where it will be in a better position to locate and extract the DIE from other sections/object files.

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

620 ↗

(On Diff #179254)

It is DWZ's reference from main debug file into shared DWZ common file - implemented by pending D40474.
I agree it needs more complicated handling than this implementation so it should not be here.

lldb/trunk/lit/SymbolFile/DWARF/array-sizes.s

+# This tests a bug where we would incorrectly parse the size of an array if that
+# size happened to match an existing DIE offset. This happened because we
+# misinterpreted that value as a reference to a DIE specifying the VLA size even
+# though the form was a data form (as it should be).
+
+# REQUIRES: lld
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj > %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: lldb-test symbols %t | FileCheck %s
+
+# CHECK: Variable{0xffffffff0000001e}, name = "X"
+# CHECK-SAME: type = {ffffffff00000033} 0x{{[0-9a-f]*}} (char [56])
+
+
+# Generated from "char X[47];"
+# The array size was modified by hand.
+
+  .text
+  .file "-"
+  .file 1 "/tmp" "<stdin>"
+  .type X,@object               # @X
+  .comm X,63,16
+  .section  .debug_str,"MS",@progbits,1
+.Linfo_string0:
+  .asciz  "clang version 8.0.0 (trunk 349604) (llvm/trunk 349608)" # string offset=0
+.Linfo_string1:
+  .asciz  "-"                     # string offset=55
+.Linfo_string2:
+  .asciz  "/tmp"                  # string offset=57
+.Linfo_string3:
+  .asciz  "X"                     # string offset=62
+.Linfo_string4:
+  .asciz  "char"                  # string offset=64
+.Linfo_string5:
+  .asciz  "__ARRAY_SIZE_TYPE__"   # string offset=69
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 37                      # DW_AT_producer
+  .byte 14                      # DW_FORM_strp
+  .byte 19                      # DW_AT_language
+  .byte 5                       # DW_FORM_data2
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte 27                      # DW_AT_comp_dir
+  .byte 14                      # DW_FORM_strp
+  .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 14                      # DW_FORM_strp
+  .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 1                       # DW_TAG_array_type
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 4                       # Abbreviation Code
+  .byte 33                      # DW_TAG_subrange_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 55                      # DW_AT_count
+  .byte 11                      # DW_FORM_data1
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 5                       # Abbreviation Code
+  .byte 36                      # DW_TAG_base_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .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 6                       # Abbreviation Code
+  .byte 36                      # DW_TAG_base_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .byte 11                      # DW_AT_byte_size
+  .byte 11                      # DW_FORM_data1
+  .byte 62                      # DW_AT_encoding
+  .byte 11                      # DW_FORM_data1
+  .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  4                       # DWARF version number
+  .long .debug_abbrev           # Offset Into Abbrev. Section
+  .byte 8                       # Address Size (in bytes)
+  .byte 1                       # Abbrev [1] 0xb:0x43 DW_TAG_compile_unit
+  .long .Linfo_string0          # DW_AT_producer
+  .short  12                      # DW_AT_language
+  .long .Linfo_string1          # DW_AT_name
+  .long 0                       # DW_AT_stmt_list
+  .long .Linfo_string2          # DW_AT_comp_dir
+  .byte 2                       # Abbrev [2] 0x1e:0x15 DW_TAG_variable
+  .long .Linfo_string3          # DW_AT_name
+  .long 51                      # DW_AT_type
+                                        # DW_AT_external
+  .byte 1                       # DW_AT_decl_file
+  .byte 1                       # DW_AT_decl_line
+  .byte 9                       # DW_AT_location
+  .byte 3
+  .quad X
+  .byte 3                       # Abbrev [3] 0x33:0xc DW_TAG_array_type
+  .long 63                      # DW_AT_type
+.Ldie:
+  .byte 4                       # Abbrev [4] 0x38:0x6 DW_TAG_subrange_type
+  .long 70                      # DW_AT_type
+  .byte .Ldie-.Lcu_begin0       # DW_AT_count - should match the offset of an existing die
+  .byte 0                       # End Of Children Mark
+  .byte 5                       # Abbrev [5] 0x3f:0x7 DW_TAG_base_type
+  .long .Linfo_string4          # DW_AT_name
+  .byte 6                       # DW_AT_encoding
+  .byte 1                       # DW_AT_byte_size
+  .byte 6                       # Abbrev [6] 0x46:0x7 DW_TAG_base_type
+  .long .Linfo_string5          # DW_AT_name
+  .byte 8                       # DW_AT_byte_size
+  .byte 7                       # DW_AT_encoding
+  .byte 0                       # End Of Children Mark
+.Ldebug_info_end0:

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

dw_addr_t DWARFFormValue::Address() const {
 
   uint32_t index_size = m_cu->GetAddressByteSize();
   dw_offset_t addr_base = m_cu->GetAddrBase();
   lldb::offset_t offset = addr_base + m_value.value.uval * index_size;
   return symbol_file->get_debug_addr_data().GetMaxU64(&offset, index_size);
 }
 
 uint64_t DWARFFormValue::Reference() const {
-  uint64_t die_offset = m_value.value.uval;
+  uint64_t value = m_value.value.uval;
   switch (m_form) {
   case DW_FORM_ref1:
   case DW_FORM_ref2:
   case DW_FORM_ref4:
   case DW_FORM_ref8:
   case DW_FORM_ref_udata:
     assert(m_cu); // CU must be valid for DW_FORM_ref forms that are compile
                   // unit relative or we will get this wrong
-    die_offset += m_cu->GetOffset();
-    break;
+    return value + m_cu->GetOffset();
+
+  case DW_FORM_ref_addr:
+  case DW_FORM_ref_sig8:
+  case DW_FORM_GNU_ref_alt:
+    return value;
 
   default:
-    break;
+    return DW_INVALID_OFFSET;
   }
-
-  return die_offset;
 }
 
 uint64_t DWARFFormValue::Reference(dw_offset_t base_offset) const {
-  uint64_t die_offset = m_value.value.uval;
+  uint64_t value = m_value.value.uval;
   switch (m_form) {
   case DW_FORM_ref1:
   case DW_FORM_ref2:
   case DW_FORM_ref4:
   case DW_FORM_ref8:
   case DW_FORM_ref_udata:
-    die_offset += base_offset;
-    break;
+    return value + base_offset;
+
+  case DW_FORM_ref_addr:
+  case DW_FORM_ref_sig8:
+  case DW_FORM_GNU_ref_alt:
+    return value;
 
   default:
-    break;
+    return DW_INVALID_OFFSET;
   }
-
-  return die_offset;
 }
 
 const uint8_t *DWARFFormValue::BlockData() const { return m_value.data; }
 
 bool DWARFFormValue::IsBlockForm(const dw_form_t form) {
   switch (form) {
   case DW_FORM_exprloc:
   case DW_FORM_block:

lldb/trunk/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp

for (size_t i = 0; i < num_atoms; ++i) {
     DWARFFormValue form_value(NULL, header_data.atoms[i].form);
 
     if (!form_value.ExtractValue(data, offset_ptr))
       return false;
 
     switch (header_data.atoms[i].type) {
     case eAtomTypeDIEOffset: // DIE offset, check form for encoding
       hash_data.offset =
-          (dw_offset_t)form_value.Reference(header_data.die_base_offset);
+          DWARFFormValue::IsDataForm(form_value.Form())
+              ? form_value.Unsigned()
+              : form_value.Reference(header_data.die_base_offset);
       break;
 
     case eAtomTypeTag: // DW_TAG value for the DIE
       hash_data.tag = (dw_tag_t)form_value.Unsigned();
       break;
 
     case eAtomTypeTypeFlags: // Flags from enum TypeFlags
       hash_data.type_flags = (uint32_t)form_value.Unsigned();