[PATCH] D137882: [DWARFLibrary] Add support to re-construct cu-index

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:868-871
+  // If we are parsing TU-index and for .debug_types section we don't need
+  // to do anything.
+  if (isParseSuccessful && TUIndex->getVersion() != 2)
+    fixupIndex(*DObj, *this, *TUIndex.get());
----------------
dblaikie wrote:
> ayermolo wrote:
> > dblaikie wrote:
> > > ayermolo wrote:
> > > > dblaikie wrote:
> > > > > Any idea what this comment/code were about checking the type index version? I don't see any reason this fixup wouldn't be relevant to DWARFv4 type unit indexes - though the fixup code would have to take the section to parse as a parameter, since it's currently hardcoded to the debug_info sections, not the debug_types sections.
> > > > I don't quite remember why. I think it was to narrow the scope.
> > > > Also don't we also need to call forEachTypesDWOSections if we want to handle .debug_types section?
> > > > Why do you see overflows in that section also?
> > > We aren't having an issue here - since it's only DWARFv4+type units, and we're using DWARFv5 these days.
> > > 
> > > I've been looking at adding DWARFv5 overflow recovery (which can be more robust than DWARFv4, since DWARFv5 can get the DWOID/type signature without needing the abbrev section) and just looking more closely at this code/seeing these quirks & wanted to understand it better.
> > > 
> > > But, yeah, maybe just not worth supporting *shrug* no worries, thanks!
> > Hopefully we will be moving to DWARF5 his year also. :D  
> > The code should work with DWARF5 + debug types, but you are right it could be made more robust by specializing and taking advantage of DWO ID being in a header. Not sure it's worth it either. 
> > 
> > Also I am not sure how often debug-types are used with DWARF5 since one has to choose between them and .debug_names. Although looking at https://reviews.llvm.org/D49420 should be easy to turn on now that llvm generates DWARF5 type units.
> > Hopefully we will be moving to DWARF5 his year also. :D
> 
> *fingers crossed*
> 
> > The code should work with DWARF5 + debug types, but you are right it could be made more robust by specializing and taking advantage of DWO ID being in a header. Not sure it's worth it either.
> 
> You mean DWARF4+debug types? It does already work with DWARF5 and debug types.
> 
> > Also I am not sure how often debug-types are used with DWARF5 since one has to choose between them and .debug_names. Although looking at https://reviews.llvm.org/D49420 should be easy to turn on now that llvm generates DWARF5 type units.
> 
> It's what we're doing at Google, at least - we're still using gdb_index, not lldb - and when we use lldb it's without any index solution, so, yeah, slow... 
> 
> Hadn't realized there was that outstanding patch for debug_names for type units. I'll have to take a look... 
oh, looked at D49420, thought that was an outstanding patch for DWARFv5 debug_names support for type units, but it's the patch that enabled debug_names but not when type units are enabled.

I think I looked at it recently and it's not quite as simple as removing the opt-out - there's some implementation complexity to address in building debug_names, also in terms of what we put in them... but yeah, needs some looking at/implementing for sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137882/new/

https://reviews.llvm.org/D137882