[lldb/DWARF] Fix handling of variables with both location and const_value attributes

[labath]

Summary

Class-level static constexpr variables can have both DW_AT_const_value
(in the "declaration") and a DW_AT_location (in the "definition")
attributes. Our code was trying to handle this, but it was brittle and
hard to follow (and broken) because it was processing the attributes in
the order in which they were found.

Refactor the code to make the intent clearer -- DW_AT_location trumps
DW_AT_const_value, and fix the bug which meant that we were not
displaying these variables properly (the culprit was the delayed parsing
of the const_value attribute due to a need to fetch the variable type.

[aprantl]

What a function :-)

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3106

Would be nice to early-exit here, too.

3229

If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would make sense...

[shafik]

LGTM with minor comments. Thank you for these fixes, they are awesome!

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3185

It might be helpful to document that DW_AT_location is taken over DW_AT_const_value and the types of cases this can show up in.

3188

const DWARFDataExtractor&?

I don't think auto adds any value here.

3213

const DWARFDataExtractor&?

3229

Why +1?

3421–3422

make_shared

[aprantl]

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3229

The NUL-terminator?

[labath]

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3106

I'll do that in a separate patch.

3229

Yeah, this is supposed to create a memory view of a C string, so it (probably) needs to include the nul terminator. But that actually speaks against the AsCStringRef function, as I wouldn't expect that one to include the null terminator.

I say "probably" because I haven't been able to actually find a producer that would produce this kind of attribute. The closest I got was gfortran, which could emit emit DW_AT_const_value for string variables. However, it used block forms for that.

To my great surprise, my synthetic test case in that patch actually worked and using a string DW_AT_const_value for char arrays in C seems somewhat reasonable, so I kept that code. However, I could be easily convinced to delete this.

lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h

enum {
     eValueTypeCStr,
     eValueTypeBlock
   };
 
   DWARFFormValue() = default;
   DWARFFormValue(const DWARFUnit *unit) : m_unit(unit) {}
   DWARFFormValue(const DWARFUnit *unit, dw_form_t form)
       : m_unit(unit), m_form(form) {}
+  const DWARFUnit *GetUnit() const { return m_unit; }
   void SetUnit(const DWARFUnit *unit) { m_unit = unit; }
   dw_form_t Form() const { return m_form; }
   dw_form_t& FormRef() { return m_form; }
   void SetForm(dw_form_t form) { m_form = form; }
   const ValueType &Value() const { return m_value; }
   ValueType &ValueRef() { return m_value; }
   void SetValue(const ValueType &val) { m_value = val; }
 

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
   var_sp = GetDIEToVariable()[die.GetDIE()];
   if (var_sp)
     return var_sp; // Already been parsed!
 
   const dw_tag_t tag = die.Tag();
   ModuleSP module = GetObjectFile()->GetModule();
 
   if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
       (tag == DW_TAG_formal_parameter && sc.function)) {
     DWARFAttributes attributes;
     const size_t num_attributes = die.GetAttributes(attributes);
     DWARFDIE spec_die;
     if (num_attributes > 0) {
       const char *name = nullptr;
       const char *mangled = nullptr;
       Declaration decl;
-      uint32_t i;
       DWARFFormValue type_die_form;
       DWARFExpression location;
       bool is_external = false;
       bool is_artificial = false;
-      bool location_is_const_value_data = false;
-      bool has_explicit_location = false;
-      DWARFFormValue const_value;
+      DWARFFormValue const_value_form, location_form;
       Variable::RangeList scope_ranges;
       // AccessType accessibility = eAccessNone;
 
-      for (i = 0; i < num_attributes; ++i) {
+      for (size_t i = 0; i < num_attributes; ++i) {
         dw_attr_t attr = attributes.AttributeAtIndex(i);
         DWARFFormValue form_value;
 
         if (attributes.ExtractFormValueAtIndex(i, form_value)) {
           switch (attr) {
           case DW_AT_decl_file:
             decl.SetFile(sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
                 form_value.Unsigned()));
if (num_attributes > 0) {
             break;
           case DW_AT_type:
             type_die_form = form_value;
             break;
           case DW_AT_external:
             is_external = form_value.Boolean();
             break;
           case DW_AT_const_value:
-            // If we have already found a DW_AT_location attribute, ignore this
-            // attribute.
-            if (!has_explicit_location) {
-              location_is_const_value_data = true;
-              // The constant value will be either a block, a data value or a
-              // string.
-              auto debug_info_data = die.GetData();
-              if (DWARFFormValue::IsBlockForm(form_value.Form())) {
-                // Retrieve the value as a block expression.
-                uint32_t block_offset =
-                    form_value.BlockData() - debug_info_data.GetDataStart();
-                uint32_t block_length = form_value.Unsigned();
-                location = DWARFExpression(
-                    module,
-                    DataExtractor(debug_info_data, block_offset, block_length),
-                    die.GetCU());
-              } else if (DWARFFormValue::IsDataForm(form_value.Form())) {
-                // Constant value size does not have to match the size of the
-                // variable. We will fetch the size of the type after we create
-                // it.
-                const_value = form_value;
-              } else if (const char *str = form_value.AsCString()) {
-                uint32_t string_length = strlen(str) + 1;
-                location = DWARFExpression(
-                    module,
-                    DataExtractor(str, string_length,
-                                  die.GetCU()->GetByteOrder(),
-                                  die.GetCU()->GetAddressByteSize()),
-                    die.GetCU());
-              }
-            }
+            const_value_form = form_value;
+            break;
+          case DW_AT_location:
+            location_form = form_value;
             break;
-          case DW_AT_location: {
-            location_is_const_value_data = false;
-            has_explicit_location = true;
-            if (DWARFFormValue::IsBlockForm(form_value.Form())) {
-              auto data = die.GetData();
-
-              uint32_t block_offset =
-                  form_value.BlockData() - data.GetDataStart();
-              uint32_t block_length = form_value.Unsigned();
-              location = DWARFExpression(
-                  module, DataExtractor(data, block_offset, block_length),
-                  die.GetCU());
-            } else {
-              DataExtractor data = die.GetCU()->GetLocationData();
-              dw_offset_t offset = form_value.Unsigned();
-              if (form_value.Form() == DW_FORM_loclistx)
-                offset = die.GetCU()->GetLoclistOffset(offset).getValueOr(-1);
-              if (data.ValidOffset(offset)) {
-                data = DataExtractor(data, offset, data.GetByteSize() - offset);
-                location = DWARFExpression(module, data, die.GetCU());
-                assert(func_low_pc != LLDB_INVALID_ADDRESS);
-                location.SetLocationListAddresses(
-                    attributes.CompileUnitAtIndex(i)->GetBaseAddress(),
-                    func_low_pc);
-              }
-            }
-          } break;
           case DW_AT_specification:
             spec_die = form_value.Reference();
             break;
           case DW_AT_start_scope:
             // TODO: Implement this.
             break;
           case DW_AT_artificial:
             is_artificial = form_value.Boolean();
if (num_attributes > 0) {
           default:
           case DW_AT_abstract_origin:
           case DW_AT_sibling:
             break;
           }
         }
       }
 
+      // Prefer DW_AT_location over DW_AT_const_value. Both can be emitted e.g.
+      // for static constexpr member variables -- DW_AT_const_value will be
+      // present in the class declaration and DW_AT_location in the DIE defining
+      // the member.
+      bool location_is_const_value_data = false;
+      bool has_explicit_location = false;
+      bool use_type_size_for_value = false;
+      if (location_form.IsValid()) {
+        has_explicit_location = true;
+        if (DWARFFormValue::IsBlockForm(location_form.Form())) {
+          const DWARFDataExtractor &data = die.GetData();
+
+          uint32_t block_offset =
+              location_form.BlockData() - data.GetDataStart();
+          uint32_t block_length = location_form.Unsigned();
+          location = DWARFExpression(
+              module, DataExtractor(data, block_offset, block_length),
+              die.GetCU());
+        } else {
+          DataExtractor data = die.GetCU()->GetLocationData();
+          dw_offset_t offset = location_form.Unsigned();
+          if (location_form.Form() == DW_FORM_loclistx)
+            offset = die.GetCU()->GetLoclistOffset(offset).getValueOr(-1);
+          if (data.ValidOffset(offset)) {
+            data = DataExtractor(data, offset, data.GetByteSize() - offset);
+            location = DWARFExpression(module, data, die.GetCU());
+            assert(func_low_pc != LLDB_INVALID_ADDRESS);
+            location.SetLocationListAddresses(
+                location_form.GetUnit()->GetBaseAddress(), func_low_pc);
+          }
+        }
+      } else if (const_value_form.IsValid()) {
+        location_is_const_value_data = true;
+        // The constant value will be either a block, a data value or a
+        // string.
+        const DWARFDataExtractor &debug_info_data = die.GetData();
+        if (DWARFFormValue::IsBlockForm(const_value_form.Form())) {
+          // Retrieve the value as a block expression.
+          uint32_t block_offset =
+              const_value_form.BlockData() - debug_info_data.GetDataStart();
+          uint32_t block_length = const_value_form.Unsigned();
+          location = DWARFExpression(
+              module,
+              DataExtractor(debug_info_data, block_offset, block_length),
+              die.GetCU());
+        } else if (DWARFFormValue::IsDataForm(const_value_form.Form())) {
+          // Constant value size does not have to match the size of the
+          // variable. We will fetch the size of the type after we create
+          // it.
+          use_type_size_for_value = true;
+        } else if (const char *str = const_value_form.AsCString()) {
+          uint32_t string_length = strlen(str) + 1;
+          location = DWARFExpression(
+              module,
+              DataExtractor(str, string_length, die.GetCU()->GetByteOrder(),
+                            die.GetCU()->GetAddressByteSize()),
+              die.GetCU());
+        }
+      }
+
       const DWARFDIE parent_context_die = GetDeclContextDIEContainingDIE(die);
       const dw_tag_t parent_tag = die.GetParent().Tag();
       bool is_static_member =
           (parent_tag == DW_TAG_compile_unit ||
            parent_tag == DW_TAG_partial_unit) &&
           (parent_context_die.Tag() == DW_TAG_class_type ||
            parent_context_die.Tag() == DW_TAG_structure_type);
 
if (num_attributes > 0) {
 
         default:
           symbol_context_scope = sc.comp_unit;
           break;
         }
       }
 
       if (symbol_context_scope) {
-        SymbolFileTypeSP type_sp(
-            new SymbolFileType(*this, GetUID(type_die_form.Reference())));
+        auto type_sp = std::make_shared<SymbolFileType>(
+            *this, GetUID(type_die_form.Reference()));
 
-        if (const_value.Form() && type_sp && type_sp->GetType())
+        if (use_type_size_for_value && type_sp->GetType())
           location.UpdateValue(
-              const_value.Unsigned(),
+              const_value_form.Unsigned(),
               type_sp->GetType()->GetByteSize(nullptr).getValueOr(0),
               die.GetCU()->GetAddressByteSize());
 
         var_sp = std::make_shared<Variable>(
             die.GetID(), name, mangled, type_sp, scope, symbol_context_scope,
             scope_ranges, &decl, location, is_external, is_artificial,
             is_static_member);
 

lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s

+## Test that we don't get confused by variables with both location and
+## const_value attributes. Such values are produced in C++ for class-level
+## static constexpr variables.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
+# RUN: %lldb %t -o "target variable A::x A::y" -o exit | FileCheck %s
+
+# CHECK-LABEL: target variable
+# CHECK: (const int) A::x = 142
+# CHECK: (const int) A::y = 242
+
+        .section        .rodata,"a",@progbits
+        .p2align        2
+_ZN1A1xE:
+        .long   142
+_ZN1A1yE:
+        .long   242
+
+        .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   8                               # DW_FORM_string
+        .byte   3                               # DW_AT_name
+        .byte   8                               # DW_FORM_string
+        .byte   0                               # EOM(1)
+        .byte   0                               # EOM(2)
+        .byte   3                               # Abbreviation Code
+        .byte   19                              # DW_TAG_structure_type
+        .byte   1                               # DW_CHILDREN_yes
+        .byte   3                               # DW_AT_name
+        .byte   8                               # DW_FORM_string
+        .byte   11                              # DW_AT_byte_size
+        .byte   11                              # DW_FORM_data1
+        .byte   0                               # EOM(1)
+        .byte   0                               # EOM(2)
+        .byte   4                               # Abbreviation Code
+        .byte   13                              # DW_TAG_member
+        .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   60                              # DW_AT_declaration
+        .byte   25                              # DW_FORM_flag_present
+        .byte   28                              # DW_AT_const_value
+        .byte   13                              # DW_FORM_sdata
+        .byte   0                               # EOM(1)
+        .byte   0                               # EOM(2)
+        .byte   5                               # Abbreviation Code
+        .byte   38                              # DW_TAG_const_type
+        .byte   0                               # DW_CHILDREN_no
+        .byte   73                              # DW_AT_type
+        .byte   19                              # DW_FORM_ref4
+        .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   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   7                               # Abbreviation Code
+        .byte   52                              # DW_TAG_variable
+        .byte   0                               # DW_CHILDREN_no
+        .byte   71                              # DW_AT_specification
+        .byte   19                              # DW_FORM_ref4
+        .byte   2                               # DW_AT_location
+        .byte   24                              # DW_FORM_exprloc
+        .byte   110                             # DW_AT_linkage_name
+        .byte   8                               # DW_FORM_string
+        .byte   0                               # EOM(1)
+        .byte   0                               # EOM(2)
+## This deliberately inverts the order of the specification and location
+## attributes.
+        .byte   8                               # Abbreviation Code
+        .byte   52                              # DW_TAG_variable
+        .byte   0                               # DW_CHILDREN_no
+        .byte   2                               # DW_AT_location
+        .byte   24                              # DW_FORM_exprloc
+        .byte   71                              # DW_AT_specification
+        .byte   19                              # DW_FORM_ref4
+        .byte   110                             # DW_AT_linkage_name
+        .byte   8                               # DW_FORM_string
+        .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 DW_TAG_compile_unit
+        .asciz  "Hand-written DWARF"            # DW_AT_producer
+        .asciz  "a.cc"                          # DW_AT_name
+        .byte   7                               # Abbrev DW_TAG_variable
+        .long   .LA__x-.Lcu_begin0              # DW_AT_specification
+        .byte   9                               # DW_AT_location
+        .byte   3
+        .quad   _ZN1A1xE
+        .asciz  "_ZN1A1xE"                      # DW_AT_linkage_name
+        .byte   8                               # Abbrev DW_TAG_variable
+        .byte   9                               # DW_AT_location
+        .byte   3
+        .quad   _ZN1A1yE
+        .long   .LA__y-.Lcu_begin0              # DW_AT_specification
+        .asciz  "_ZN1A1yE"                      # DW_AT_linkage_name
+        .byte   3                               # Abbrev DW_TAG_structure_type
+        .asciz  "A"                             # DW_AT_name
+        .byte   1                               # DW_AT_byte_size
+.LA__x:
+        .byte   4                               # Abbrev DW_TAG_member
+        .asciz  "x"                             # DW_AT_name
+        .long   .Lconst_int-.Lcu_begin0         # DW_AT_type
+                                                # DW_AT_declaration
+        .sleb128 147                            # DW_AT_const_value
+.LA__y:
+        .byte   4                               # Abbrev DW_TAG_member
+        .asciz  "y"                             # DW_AT_name
+        .long   .Lconst_int-.Lcu_begin0         # DW_AT_type
+                                                # DW_AT_declaration
+        .sleb128 247                            # DW_AT_const_value
+        .byte   0                               # End Of Children Mark
+.Lconst_int:
+        .byte   5                               # Abbrev DW_TAG_const_type
+        .long   .Lint-.Lcu_begin0               # DW_AT_type
+.Lint:
+        .byte   6                               # Abbrev 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_end0: