[Lldb-commits] [PATCH] D52375: [WIP] Support multiple compile units per OSO entry in SymbolFileDWARFDebugMap

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:431
     const size_t num_ranges =
-        die->GetAttributeAddressRanges(dwarf, this, ranges, false);
+        die->GetAttributeAddressRanges(dwarf, this, ranges, check_hi_lo_pc);
     if (num_ranges > 0) {
----------------
sgraenitz wrote:
> clayborg wrote:
> > sgraenitz wrote:
> > > @JDevlieghere Thanks for your feedback!
> > > 
> > > @everyone:
> > > This parameter is, actually, one of the key questions in this review: If there is no DW_AT_range attribute for the compile unit, can we safely fall back to DW_AT_low/high_pc?
> > I have seen DWARF where the DW_AT_ranges is incorrect and I have seen cases where the low pc is just set to zero and the high pc is set to the max address. So you might have many overlapping ranges between different CUs in the latter case. We might need to be more vigilant with high/low pc values though and that was the reason that we previously ignored high/low pc values. If the stuff is missing, it should index the DWARF and generate the address ranges table manually right? What was the reasoning behind this change?
> I had a closer look at the individual fallbacks and you are right. After fixing `SymbolFileDWARF::ParseCompileUnit()` we have the correct `CompileUnit` pointers in `DWARFUnit::m_user_data` for all CUs. Thus, the below `die->BuildAddressRangeTable(dwarf, this, debug_aranges);` now correctly constructs the ranges! Great, with this we can keep `check_hi_lo_pc=false`.
> 
> I didn't notice this while testing, because of the following issue that kept my tests failing: 2 of my LTO object's CUs have no code remaining after optimization, so we step into the next fallback assuming 'a line tables only situation' and eventually call `AddOSOARanges()`, which adds to `debug_aranges` all entries of the CU's FileRangeMap with a zero offset.
> 
> Thus, my `debug_aranges` had 28000 entries too much and most requests would continue to return `0` as before. For now I commented out the call below.
> 
> **What do you think is a good way to avoid this for empty CUs?
> IIUC they should have no DW_AT_low/high_pc right? :-D**
+1 to @clayborg's suggestion here that we either use AT_ranges, or build up the ranges by parsing the low/high pc's in subprograms. ISTM that it'd be acceptable to implement the fallback path as a follow-up, provided that doing so isn't a regression.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:489
+      } //else
+        //debug_map_sym_file->AddOSOARanges(dwarf, debug_aranges);
     }
----------------
Could you leave an in-source comment explaining why this is commented out?


https://reviews.llvm.org/D52375