[Lldb-commits] [PATCH] D12238: Add support for DW_FORM_GNU_[addr, str]_index

[Tamas Berghammer via lldb-commits]

tberghammer added inline comments.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:729
@@ -727,3 +728,3 @@
                     if (attributes.ExtractFormValueAtIndex(m_dwarf2Data, i, form_value))
-                        name = form_value.AsCString(debug_str);
+                        name = form_value.AsCString(debug_str, debug_str_offsets);
                     break;
----------------
clayborg wrote:
> Don't we need to pass both debug_str and debug_str_offsets here? Other places seem to pass both like on DWARFDebugInfoEntry.cpp:826
Both of them is passed in here. The only difference is that they are pre-fetched in line 662-663.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:745
@@ -743,3 +744,3 @@
                     if (attributes.ExtractFormValueAtIndex(m_dwarf2Data, i, form_value))
-                        mangled_cstr = form_value.AsCString(debug_str);                        
+                        mangled_cstr = form_value.AsCString(debug_str, debug_str_offsets);
                     break;
----------------
clayborg wrote:
> Don't we need to pass both debug_str and debug_str_offsets here? Other places seem to pass both like on DWARFDebugInfoEntry.cpp:826
> 
Both of them is passed in here. The only difference is that they are pre-fetched in line 662-663.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:190
@@ -189,7 +189,3 @@
         case DW_FORM_data8:     m_value.value.uval = data.GetU64(offset_ptr);                           break;
-        case DW_FORM_string:    m_value.value.cstr = data.GetCStr(offset_ptr);
-                                // Set the string value to also be the data for inlined cstr form values only
-                                // so we can tell the difference between DW_FORM_string and DW_FORM_strp form
-                                // values;
-                                m_value.data = (const uint8_t*)m_value.value.cstr;                      break;
+        case DW_FORM_string:    m_value.value.cstr = data.GetCStr(offset_ptr);                          break;
         case DW_FORM_exprloc:
----------------
clayborg wrote:
> We we not longer need "m_value.data" set to the cstring value to tell the difference between DW_FORM_string and DW_FORM_strp? This might have been left over from the code this originated from and might be able to be removed, but I just wanted to verify that you intended to remove this.
We store the information in the m_form field, so we don't have to rely on this magic anymore (I don't see why we needed it at the first place).

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFFormValue.h:68
@@ -68,1 +67,3 @@
+                                  const lldb_private::DWARFDataExtractor* debug_str_offsets_data_ptr) const;
+    dw_addr_t           Address(const lldb_private::DWARFDataExtractor* debug_addr_data_ptr) const;
     bool                SkipValue(const lldb_private::DWARFDataExtractor& debug_info_data, lldb::offset_t *offset_ptr) const;
----------------
clayborg wrote:
> Should be pass in a lldb::addr_t base_addr in case the DW_FORM is one where we need to add to the low_pc? Then code like this:
> 
> ```
>         dw_form_t form = form_value.Form();
>         if (form == DW_FORM_addr || form == DW_FORM_GNU_addr_index)
>             return form_value.Address(&dwarf2Data->get_debug_addr_data());
>         
>         // DWARF4 can specify the hi_pc as an <offset-from-lowpc>
>         return lo_pc + form_value.Unsigned();
> ```
> 
> Becomes:
> 
> ```
>             return form_value.Address(&dwarf2Data->get_debug_addr_data(), lo_pc);
> ```
> 
> The "base_addr" could default to LLDB_INVALID_ADDRESS and if we get a relative address we should assert in debug builds using LLDB_ASSERT.
> 
We have a dedicated function for that called DWARFDebugInfoEntry::GetAttributeHighPC. The purpose of this function is to fetch a value where the attribute encoding value class is "address" (http://www.dwarfstd.org/doc/DWARF4.pdf page 155)


http://reviews.llvm.org/D12238