[Lldb-commits] [PATCH] D12291: Add split dwarf support to SymbolFileDWARF

[Tamas Berghammer via lldb-commits]

tberghammer added inline comments.

================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.cpp:34-44
@@ +33,13 @@
+
+DIERef::DIERef(const DWARFFormValue& form_value) :
+    cu_offset(DW_INVALID_OFFSET),
+    die_offset(DW_INVALID_OFFSET)
+{
+    if (form_value.IsValid())
+    {
+        if (form_value.GetCompileUnit())
+            cu_offset = form_value.GetCompileUnit()->GetOffset();
+        die_offset = form_value.Reference();
+    }
+}
+
----------------
clayborg wrote:
> For DWO files, won't every compile unit have offset 0x0000000b? Or do you use the actual compile unit DIE from the main executable?
> 
> I have doubts this will work for DWARF in .o files. We used to encode the compile unit index in the upper 32 bits because all of our compile units have a dw_offset_t that is 0xb..
The concept of DIERef is to contain the offset of the compile unit in the main object file so based on a DIERef and a SymbolFileDWARF fro the main object file we can find the compile unit and the DIE. In case of dsym it changes a bit with storing the symbol file index in the cu_offset filed instead.

I fixed the implementation to set it up properly in case of dwo files

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1025-1036
@@ +1024,14 @@
+    {
+        if (GetAttributeValue(dwarf2Data, cu, DW_AT_specification, form_value) ||
+            GetAttributeValue(dwarf2Data, cu, DW_AT_abstract_origin, form_value))
+        {
+            DWARFDIE die = const_cast<DWARFCompileUnit*>(cu)->GetDIE(form_value.Reference());
+            if (die)
+                return die.GetDIE()->GetAttributeValue(die.GetDWARF(),
+                                                       die.GetCU(),
+                                                       attr,
+                                                       form_value,
+                                                       end_attr_offset_ptr,
+                                                       false);
+        }
+    }
----------------
clayborg wrote:
> This will fail if a DIE can have both a DW_AT_specification and a DW_AT_abstract_origin. Not sure if that can happen. If it can, we will need to check each one individually. Probably best to just check both individually.
We haven't checked both of it previously either and I don't think we will ever have both a DW_AT_specification and a DW_AT_abstract_origin, but changed it to check both for just in case.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1039-1055
@@ -985,1 +1038,19 @@
+
+    if (!dwo_symbol_file)
+        return 0;
+
+    DWARFCompileUnit* dwo_cu = dwo_symbol_file->GetCompileUnit();
+    if (!dwo_cu)
+        return 0;
+
+    DWARFDIE dwo_cu_die = dwo_cu->GetCompileUnitDIEOnly();
+    if (!dwo_cu_die.IsValid())
+        return 0;
+
+    return dwo_cu_die.GetDIE()->GetAttributeValue(dwo_symbol_file,
+                                                  dwo_cu,
+                                                  attr,
+                                                  form_value,
+                                                  end_attr_offset_ptr,
+                                                  check_specification_or_abstract_origin);
 }
----------------
clayborg wrote:
> Don't you want to only do all of this code if:
> ```
> if (Tag() == DW_TAG_compile_unit)
> ```
Yes I do.

The beginning of the function (line 987-995) handles the case for non DW_TAG_compile_unit with forwarding the query to the right compile unit and this part of the code is to handle the case when some data about the compile unit is in the main object file and some data is in the dwo file. This can't happen for any other DIE.


http://reviews.llvm.org/D12291