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

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Looks good, a few changes needed.

We might consider changing FormValue::AsCString() to take a "SymbolFileDWARF*" instead of the data extractor for .debug_str and .debug_str_offsets. This way it would be only one parameter and it would clean up the code. It would help errors like on DWARFCompileUnit.cpp:729 and DWARFCompileUnit.cpp:745 where both were not passed.

Other than that we might consider modifying FormValue::Address(...) as suggested in the inlined 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;
----------------
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

================
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;
----------------
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


================
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:
----------------
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.

================
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;
----------------
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.



http://reviews.llvm.org/D12238