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

[Tamas Berghammer via lldb-commits]

tberghammer added a comment.

In http://reviews.llvm.org/D12291#232191, @clayborg wrote:

> I also question why Symbo
>
> In http://reviews.llvm.org/D12291#231523, @tberghammer wrote:
>
> > In the current version of the patch the compile units in the main object file hands out only the compile unit DIE with the information what is available in the main object file. I considered the other approach (hand out all DIEs by the DWARF compile unit in the main object file) but I dropped it for the following reasons:
> >
> > - If we hand out DIEs from a dwo symbol file then each DIE have to store a pointer to the symbol file (or compile unit) it belongs to what is a significant memory overhead (we have more DIEs then Symbols) if we ever want to store all DIE in memory. Even worse is that we have to change all name to DIE index to contain a pointer (compile unit / dwo symbol file) and an offset to find the DIE belongs to (compared to just an offset now) what is more entry then the number DIEs.
>
>
> Can't we just store the SymbolFile inside the compile unit? We always need the compile unit to really do anything with a DIE anyway. We currently store the DWO file inside the compile unit so it seems that we could just store it once in the compile unit and avoid any extra cost.


In the name to DIE indexes (in SymbolFileDWARF) currently we store only a DIE offset and we find the compile unit based on the fact that the DIE should be inside the range of the compile unit. The compile units leave in the dwo symbol files all start at address 0 so just a DIE offset isn't enough to find the compile unit. We can store the offset in 4 byte (we already do it, but I am not sure it is a good idea) and the compile unit index in another 4 byte what isn't a major overhead, but it can matter for large inferiors. Storing the symbol file  in the DIE might be avoidable but then the DIE have to ask the compile unit for the correct symbol file when somebody queries it for an attribute (we don't want the caller of the GetAttribute* function to know about dwo files).

> 

> 

> > - In an average debug session run from an IDE the user usually sets the breakpoints based on file name + line number, display some stack traces, some variables and do some stepping. If we can index each dwo file separately then we can handle all of these features without parsing the full debug info what can give us some significant speed benefits at debugger startup time.

> 

> 

> I don't see how we can ever just index one DWO file? If we index one, we must index them all within an executable otherwise the index will be incomplete. If you set a file + line breakpoint, you can't rely on the file matching the compile unit because there could be inlined functions so you would always need to index all of them. Likewise with setting a breakpoint by function name, you will need to index all DWO files.


I made a few measurements a few weeks ago and setting a file + line breakpoint re

> Maybe we can:

> 

> - Have a new class that we hand out for a DIE, maybe named DWARFDIE that contains:

> 

>   ``` class DWARFDIE { DWARFCompileUnit *m_cu; DWARFDebugInfoEntry *m_die; }; ```

> 

>   Then change all of the places we currently use a "DWARFCompileUnit *cu, DWARFDebugInfoEntry* die" (we always pass them around together) to use a DWARFDIE instead. This allows us to store the DIEs efficiently, yet pass them around in a slightly larger container for usage. This would allow our memory usage to stay very close to where it is (an extra pointer in the compile unit).

> 

>   Then we modify DWARFCompileUnit to store the "SymbolFileDWARF *" that the compile unit comes from. We can still store the DWO file in the compile unit as well as you are already doing, we would just need to add a "SymbolFileDWARF *m_dwarf;" member variable for the non DWO case (and also for digging up the DW_TAG_compile_unit attributes that aren't in the DWO DW_TAG_compile_unit).

> 

>   Then we just make sure that all code that hands out DIEs actually hands out DWARFDIE instances instead of returning a "DWARFDebugInfoEntry *" and also having an out parameter that fills in the compile unit.

> 

>   Thoughts?



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:348
@@ -292,1 +347,3 @@
+    }
+}
 
----------------
abidh wrote:
> You are not using the DW_AT_GNU_dwo_id. Is this intentional or an oversight?
I just forgot to check if (fixed).

================
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;
----------------
clayborg wrote:
> 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
> }
> ```
Nice catch

================
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();
----------------
clayborg wrote:
> GetAttributeValue(...) should just "do the right thing" and look into the DWO file if needed. Then this change isn't needed.
I have to return a SymbolFileDWARF* from GetAttributeValue so Address and String fetching know where it have to find the referenced sections, but it made the code significantly simpler.

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3059-3061
@@ -2984,1 +3058,5 @@
+
+    DWARFDebugInfo* info = DebugInfo();
+    if (info == nullptr)
+        return 0;
 
----------------
clayborg wrote:
> 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?
You are right. I removed it


http://reviews.llvm.org/D12291