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

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo 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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137882