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

Greg Clayton via lldb-commits lldb-commits at lists.llvm.org
Mon Aug 24 13:40:38 PDT 2015


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

I see the need for a lot of this, but I feel like there are way too many places where we do this:

  SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
  if (dwo_symbol_file)
      return dwo_symbol_file->XXXX (...);

I would like us to more cleanly abstract ourselves from SymbolFileDWARFDwo. Ideally we wouldn't even see "SymbolFileDWARFDwo" anywhere inside SymbolFileDWARF because it has been abstracted behind DWARFCompileUnit and DWARFDebugInfoEntry and any other low level classes that need to know about them.

So I would like to see if the following is possible:

- DWARFCompileUnit should return the DIEs from the DWO file when DWARFCompileUnit::GetCompileUnitDIEOnly() or DWARFCompileUnit::DIE() is called.
- Any code that was doing the pattern from above that was calling dwarf_cu->GetDwoSymbolFile() and deferring to the DWO symbol file should just work normally if the correct DIEs are being handed out.
- All references to SymbolFileDWARFDwo should be removed from SymbolFileDWARF if we abstract things correctly.


================
Comment at: include/lldb/Symbol/ObjectFile.h:372
@@ -371,3 +371,3 @@
     virtual SectionList *
-    GetSectionList ();
+    GetSectionList (bool update_module_section_list = true);
 
----------------
Why is this needed? Can you clarify. I don't like this change and would rather avoid it if possible.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:16
@@ -15,2 +15,3 @@
 #include "SymbolFileDWARF.h"
+#include "SymbolFileDWARFDwo.h"
 
----------------
This shouldn't be needed here, just a forward declaration for SymbolFileDWARFDwo should do and then include this in DWARFCompileUnit.cpp. No one outside of this class should need to know anything about SymbolFileDWARFDwo. It should be an implementation detail that only DWARFCompileUnit and DWARFDebugInfoEntry need to know about.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:208-214
@@ -202,1 +207,9 @@
+    
+    SymbolFileDWARFDwo*
+    GetDwoSymbolFile()
+    {
+        // Extract the compile unit DIE as that one contains the dwo symbol file information
+        ExtractDIEsIfNeeded(true);
+        return m_dwo_symbol_file.get();
+    }
 
----------------
Make protected and hide this from anyone but DWARFCompileUnit and DWARFDebugInfoEntry.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:330
@@ +329,3 @@
+                                dw_addr_t base_address = form_value.Address(dwarf2Data);
+                                ((DWARFCompileUnit*)cu)->SetBaseAddress(base_address);
+                            }
----------------
I don't think the cast is needed anymore now that "cu" isn't const.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:806-818
@@ -802,10 +805,15 @@
                 case DW_AT_high_pc:
-                    hi_pc = form_value.Unsigned();
-                    if (form_value.Form() != DW_FORM_addr)
+                    if (form_value.Form() == DW_FORM_addr ||
+                        form_value.Form() == DW_FORM_GNU_addr_index)
                     {
+                        form_value.Address(dwarf2Data);
+                    }
+                    else
+                    {
+                        hi_pc = form_value.Unsigned();
                         if (lo_pc == LLDB_INVALID_ADDRESS)
                             do_offset = hi_pc != LLDB_INVALID_ADDRESS;
                         else
                             hi_pc += lo_pc; // DWARF 4 introduces <offset-from-lo-pc> to save on relocations
                     }
                     break;
----------------
Shouldn't this be:

```
if (form_value.Form() == DW_FORM_addr || form_value.Form() == DW_FORM_GNU_addr_index)
    hi_pc = form_value.Address(dwarf2Data);
else
    hi_pc = form_value.Unsigned();
if (hi_pc != LLDB_INVALID_ADDRESS)
{
    if (lo_pc == LLDB_INVALID_ADDRESS)
        do_offset = hi_pc != LLDB_INVALID_ADDRESS;
    else
        hi_pc += lo_pc; // DWARF 4 introduces <offset-from-lo-pc> to save on relocations
}
```

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1437-1438
@@ -1384,3 +1436,4 @@
     DWARFFormValue form_value;
-    if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+    if (GetAttributeValue(dwarf2Data, cu, attr, form_value) ||
+        GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, attr, form_value))
         return form_value.Unsigned();
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO file if needed. Then this change isn't needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1458-1459
@@ -1404,3 +1457,4 @@
     DWARFFormValue form_value;
-    if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+    if (GetAttributeValue(dwarf2Data, cu, attr, form_value) ||
+        GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, attr, form_value))
         return form_value.Signed();
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO file if needed. Then this change isn't needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1480-1481
@@ -1425,3 +1479,4 @@
     DWARFFormValue form_value;
-    if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+    if (GetAttributeValue(dwarf2Data, cu, attr, form_value) ||
+        GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, attr, form_value))
         return form_value.Reference();
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO file if needed. Then this change isn't needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1494-1515
@@ +1493,24 @@
+) const
+{
+    DWARFFormValue form_value;
+    if (GetAttributeValue(dwarf2Data, cu, attr, form_value))
+        return form_value.Address(dwarf2Data);
+    
+    if (m_tag != DW_TAG_compile_unit)
+        return fail_value;
+        
+    SymbolFileDWARFDwo* dwo_sym_file = cu->GetDwoSymbolFile();
+    if (!dwo_sym_file)
+        return fail_value;
+    
+    DWARFCompileUnit* dwo_cu = dwo_sym_file->GetCompileUnit();
+    if (!dwo_cu)
+        return fail_value;
+
+    const DWARFDebugInfoEntry* dwo_cu_die = dwo_cu->GetCompileUnitDIEOnly();
+    if (!dwo_cu_die)
+        return fail_value;
+
+    return dwo_cu_die->GetAttributeValueAsAddress(dwo_sym_file, dwo_cu, attr, fail_value);
+}
+
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO file if needed. This code will change a bit after that, but the check for the DWO file shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:1535-1536
@@ -1448,3 +1534,4 @@
     DWARFFormValue form_value;
-    if (GetAttributeValue(dwarf2Data, cu, DW_AT_high_pc, form_value))
+    if (GetAttributeValue(dwarf2Data, cu, DW_AT_high_pc, form_value) ||
+        GetAttributeValueForDwoCompileUnit(dwarf2Data, cu, DW_AT_high_pc, form_value))
     {
----------------
GetAttributeValue(...) should just "do the right thing" and look into the DWO file if needed. Then this change isn't needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:371-384
@@ -369,7 +370,16 @@
             return 0;
-        GetTypes (dwarf_cu,
-                  dwarf_cu->DIE(),
-                  dwarf_cu->GetOffset(),
-                  dwarf_cu->GetNextCompileUnitOffset(),
-                  type_mask,
-                  type_set);
+
+        SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+        if (dwo_symbol_file)
+        {
+            dwarf_cu = dwo_symbol_file->GetCompileUnit();
+            dwo_symbol_file->GetTypes (dwarf_cu,
+                                       dwarf_cu->DIE(),
+                                       0,
+                                       UINT32_MAX,
+                                       type_mask,
+                                       type_set);
+        }
+        else
+        {
+            GetTypes (dwarf_cu,
----------------
Hopefully we don't need this if we abstract things correctly so that the DWARFCompileUnit hands out the correct DIEs from the DWO file and SymbolFileDWARF shouldn't need to know anything about DWO files.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:404-417
@@ -387,7 +403,16 @@
                 {
-                    GetTypes (dwarf_cu,
-                              dwarf_cu->DIE(),
-                              0,
-                              UINT32_MAX,
-                              type_mask,
-                              type_set);
+                    SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+                    if (dwo_symbol_file)
+                    {
+                        dwarf_cu = dwo_symbol_file->GetCompileUnit();
+                        dwo_symbol_file->GetTypes (dwarf_cu,
+                                                   dwarf_cu->DIE(),
+                                                   0,
+                                                   UINT32_MAX,
+                                                   type_mask,
+                                                   type_set);
+                    }
+                    else
+                    {
+                        GetTypes (dwarf_cu,
+                                  dwarf_cu->DIE(),
----------------
Hopefully we don't need this if we abstract things correctly so that the DWARFCompileUnit hands out the correct DIEs from the DWO file and SymbolFileDWARF shouldn't need to know anything about DWO files.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1069-1074
@@ -1038,1 +1068,8 @@
                             
+                            SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+                            if (dwo_symbol_file)
+                            {
+                                DWARFCompileUnit* dwo_dwarf_cu = dwo_symbol_file->GetCompileUnit();
+                                dwo_dwarf_cu->SetUserData(cu_sp.get());
+                            }
+                            
----------------
Shouldn't:

```
const DWARFDebugInfoEntry* cu_die = dwarf_cu->GetCompileUnitDIEOnly ();
```

already return the right cu_die? Should clients of DWARFCompileUnit be required to do this kind of very specific check? I really want this to be hidden from us by abstracting this kind of thing inside DWARFCompileUnit.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1157-1159
@@ -1119,1 +1156,5 @@
     {
+        SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+        if (dwo_symbol_file)
+            return dwo_symbol_file->ParseFunctionBlocks(sc);
+
----------------
Why is this necessary? If we hand out the correct DIE from the DWO file in the first place, this should just work with no modifications right?

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1626-1631
@@ -1584,1 +1625,8 @@
     {
+        // Ask the child dwo files if any of them know about this type
+        for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+        {
+            if (dwo_symbol_file && dwo_symbol_file->HasForwardDeclForClangType(clang_type))
+                return dwo_symbol_file->CompleteType(clang_type);
+        }
+
----------------
It would be nice if this weren't linear. I think we have the same problem int SymbolFileDWARFDebugMap, but not being linear would be nice at some point.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1922-1925
@@ -1874,2 +1921,6 @@
                     {
+                        SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+                        if (dwo_symbol_file)
+                            return dwo_symbol_file->ResolveSymbolContext(so_addr, resolve_scope, sc);
+
                         resolved |= eSymbolContextCompUnit;
----------------
It would be nice to abstract this behind DWARFCompileUnit. Below we end up calling dwarf_cu->LookupAddress(...). This seems like a better place to do this kind of thing...

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2281-2286
@@ -2225,1 +2280,8 @@
 
+        for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+            dwo_symbol_file->FindGlobalVariables(name,
+                                                 namespace_decl,
+                                                 true /* append */,
+                                                 max_matches,
+                                                 variables);
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2405-2410
@@ -2343,1 +2404,8 @@
+
+        for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+            dwo_symbol_file->FindGlobalVariables(regex,
+                                                 true /* append */,
+                                                 max_matches,
+                                                 variables);
+
         m_global_index.Find (regex, die_offsets);
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3017-3023
@@ -2949,1 +3016,9 @@
+
+        for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+            dwo_symbol_file->FindFunctions(name,
+                                           namespace_decl,
+                                           name_type_mask,
+                                           include_inlines,
+                                           true /* append */,
+                                           sc_list);
     }
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3059-3061
@@ -2984,1 +3058,5 @@
+
+    DWARFDebugInfo* info = DebugInfo();
+    if (info == nullptr)
+        return 0;
 
----------------
If .debug_info wasn't there, then SymbolFileDWARF wouldn't have been instantiated. Is this truly needed? Did something change when DWO files are now used?

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3086-3087
@@ -3007,1 +3085,4 @@
+
+        for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+            dwo_symbol_file->FindFunctions(regex, include_inlines, true, sc_list);
     }
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3149-3155
@@ -3066,1 +3148,9 @@
+
+        for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+            dwo_symbol_file->FindTypes(sc,
+                                       name,
+                                       namespace_decl,
+                                       true /* append */,
+                                       max_matches,
+                                       types);
     }
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3265-3270
@@ -3175,1 +3264,8 @@
 
+            for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+            {
+                namespace_decl = dwo_symbol_file->FindNamespace(sc, name, parent_namespace_decl);
+                if (namespace_decl)
+                    return namespace_decl;
+            }
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3723-3728
@@ -3655,1 +3722,8 @@
+
+                for (SymbolFileDWARFDwo* dwo_symbol_file : m_dwo_symbol_files)
+                {
+                    type_sp = dwo_symbol_file->FindDefinitionTypeForDWARFDeclContext(dwarf_decl_ctx);
+                    if (type_sp)
+                        return type_sp;
+                }
                 
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3909-3911
@@ -3834,1 +3908,5 @@
     {
+        SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+        if (dwo_symbol_file)
+            return dwo_symbol_file->ParseFunctionBlocks(sc);
+
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3934-3936
@@ -3855,1 +3933,5 @@
     {
+        SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+        if (dwo_symbol_file)
+            return dwo_symbol_file->ParseTypes(sc);
+        
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3975-3978
@@ -3894,1 +3974,6 @@
+
+            SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+            if (dwo_symbol_file)
+                return dwo_symbol_file->ParseVariablesForContext (sc);
+
             const DWARFDebugInfoEntry *function_die = dwarf_cu->GetDIEPtr(sc.function->GetID());
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3997-3999
@@ -3912,1 +3996,5 @@
+            
+            SymbolFileDWARFDwo* dwo_symbol_file = dwarf_cu->GetDwoSymbolFile();
+            if (dwo_symbol_file)
+                return dwo_symbol_file->ParseVariablesForContext (sc);
 
----------------
Not sure why this is needed? If each DWARFCompileUnit hands out the correct DIEs, then Index() will point us to the right things and this shouldn't be needed.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:66-67
@@ -65,2 +65,4 @@
 
+class SymbolFileDWARFDwo;
+
 class SymbolFileDWARF : public lldb_private::SymbolFile, public lldb_private::UserID
----------------
Hopefully we don't need this if we abstract things correctly so that the DWARFCompileUnit hands out the correct DIEs from the DWO file and SymbolFileDWARF shouldn't need to know anything about DWO files.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:419-423
@@ -418,1 +418,7 @@
+    
+    void
+    AddDwoSymbolFile(SymbolFileDWARFDwo* dwo_symbol_file)
+    {
+        m_dwo_symbol_files.push_back(dwo_symbol_file);
+    }
 
----------------
Hopefully we don't need this if we abstract things correctly so that the DWARFCompileUnit hands out the correct DIEs from the DWO file and SymbolFileDWARF shouldn't need to  know anything about DWO files.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:478
@@ -471,2 +477,3 @@
     ClangTypeToDIE m_forward_decl_clang_type_to_die;
+    std::vector<SymbolFileDWARFDwo*> m_dwo_symbol_files;
 };
----------------
Hopefully we don't need this if we abstract things correctly so that the DWARFCompileUnit hands out the correct DIEs from the DWO file and SymbolFileDWARF shouldn't need to know anything about DWO files.

================
Comment at: source/Symbol/ObjectFile.cpp:604-625
@@ -603,15 +603,23 @@
 
 SectionList *
-ObjectFile::GetSectionList()
+ObjectFile::GetSectionList(bool update_module_section_list)
 {
     if (m_sections_ap.get() == nullptr)
     {
-        ModuleSP module_sp(GetModule());
-        if (module_sp)
+        if (update_module_section_list)
         {
-            lldb_private::Mutex::Locker locker(module_sp->GetMutex());
-            CreateSections(*module_sp->GetUnifiedSectionList());
+            ModuleSP module_sp(GetModule());
+            if (module_sp)
+            {
+                lldb_private::Mutex::Locker locker(module_sp->GetMutex());
+                CreateSections(*module_sp->GetUnifiedSectionList());
+            }
+        }
+        else
+        {
+            SectionList unified_section_list;
+            CreateSections(unified_section_list);
         }
     }
     return m_sections_ap.get();
 }
----------------
Can you explain why this is needed?


http://reviews.llvm.org/D12291





More information about the lldb-commits mailing list