[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/DWARFCompileUnit.cpp:316-328
@@ +315,15 @@
+
+    const char* dwo_name = cu_die.GetAttributeValueAsString(m_dwarf2Data,
+                                                            this,
+                                                            DW_AT_GNU_dwo_name,
+                                                            nullptr);
+    if (!dwo_name)
+        return;
+    
+    const char* comp_dir = cu_die.GetAttributeValueAsString(m_dwarf2Data,
+                                                            this,
+                                                            DW_AT_comp_dir,
+                                                            nullptr);
+    if (!comp_dir)
+        return;
+
----------------
clayborg wrote:
> Is DW_AT_GNU_dwo_name always a relative path?
I haven't seen absolute path in it so far, but it can be absolute. Added code to handle it.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:660
@@ -564,3 +659,3 @@
         {
-            return m_dwarf2Data->DebugInfo()->GetDIE (die_offset);
+            return m_dwarf2Data->DebugInfo()->GetDIE (DIERef(die_offset));
         }
----------------
clayborg wrote:
> Don't we need the compile unit to be encoded into the DIERef here?
No, we don't want to specify the compile unit here. We already checked that the specified DIE offset isn't belongs to the current compile unit. In this case we should try to get a DIE based on offset from the current symbol file. This can happen when a DIE refers to a value in a different compile unit inside the same file based on DIE offset

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:161
@@ -145,3 +160,3 @@
                 else
-                    return DWARFDIE(dwarf->DebugInfo()->GetCompileUnitContainingDIE(block_die->GetOffset()), block_die);
+                    return DWARFDIE(dwarf->DebugInfo()->GetCompileUnitContainingDIE(DIERef(cu->GetOffset(), block_die->GetOffset())), block_die);
             }
----------------
clayborg wrote:
> We probably should swift m_die->LookupAddress to take a "DWARFDIE *function_die, DWARFDIE *block_die" instead of "DWARFDebugInfoEntry* function_die, DWARFDebugInfoEntry* block_die". Then we can avoid doing the manual combination of CU + offset here.
I would prefer to leave it as it is now because we have to do the manual combination at some point (because the returned DIE might be in a different CU then the current DIE) and moving it into LookupAddress will make that function even more complicated.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp:184
@@ +183,3 @@
+            assert ((id&0xffffffff00000000ull) == 0 || m_cu->GetID() == 0);
+            id |= ((lldb::user_id_t)m_cu->GetID()) << 32;
+        }
----------------
clayborg wrote:
> The DWARFCompileUnit::GetID() already shifts the cu_offset over by 32, so we will just lose the compile unit bits here.
SymbolFileDWARF does it, but DWARFCompileUnit isn't. With shifting by 32 we lose the SymbolFileDWARF id, but that one isn't considered to be part of the DIE ID.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1250
@@ -1180,2 +1249,3 @@
     dw_offset_t end_addr_offset = DW_INVALID_OFFSET;
-    const dw_offset_t attr_offset = GetAttributeValue(dwarf2Data, cu, attr, form_value, &end_addr_offset);
+    SymbolFileDWARF* attribute_symbol_file = nullptr;
+    const dw_offset_t attr_offset = GetAttributeValue(dwarf2Data,
----------------
clayborg wrote:
> Not sure what you were thinking here? You init this with nullptr and then try to use it below? Did you mean to init this with dwarf2Data? If so then we don't need this? Otherwise, please fix
The function isn't used at all now, so I just removed it completely.

(I made the change here to fetch the block data from the correct symbol file, but one of the merge made it broken)


http://reviews.llvm.org/D12291