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

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:439
+        die->GetAttributeAddressRanges(dwarf, this, ranges, check_hi_lo_pc);
+
     if (num_ranges > 0) {
----------------
clayborg wrote:
> Empty CUs won't have any children. Easy to skip by getting the CU die and doing:
> ```
> if (!die->HasChildren())
> ```
> 
Sorry if that was too unspecific. With "no code left" I meant it has no instructions. The DIE does have child DIEs for declarations (e.g. TAG_namespace, TAG_enumeration_type, TAG_imported_declaration), but nothing that would form a range.

I could traverse the DWARF and filter for DIEs with instructions (e.g. TAG_subprogram), but it seems pretty much what `die->BuildAddressRangeTable()` does already.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:488-489
         }
-      } else
-        debug_map_sym_file->AddOSOARanges(dwarf, debug_aranges);
+      } //else
+        //debug_map_sym_file->AddOSOARanges(dwarf, debug_aranges);
     }
----------------
clayborg wrote:
> revert if the "if (!die->hasChildren())" trick works.
> revert if the "if (!die->hasChildren())" trick works.
As noted above, unfortunately, it doesn't. The exploration below got quite verbose. 

It looks like a conceptual clash to me: For OSO debug map entries with multiple CUs, we can't map between symtab entries and CUs. Thus, each CU's FileRangeMap will be populated from the entire symtab range of that OSO debug map entry and the call to `AddOSOARanges()` here will add all of that to `debug_aranges` for each CU again.

**We should have one FileRangeMap per OSO debug map entry and not one per CU.**

Even if I fixed `AddOSOARanges()` to write the current CU offset instead of `dwarf2Data->GetID()` and FileRangeMap existed per OSO debug map entry (or the huge number of duplicates was acceptable), it would still add entries from other CUs of that OSO debug map entry. This cannot work with the current approach in `SymbolFileDWARF::ResolveSymbolContext()` because we will have #CUs candidates here, but we only look for a single result:
```
const dw_offset_t cu_offset =
    debug_info->GetCompileUnitAranges().FindAddress(file_vm_addr);
```

In order to keep the current `AddOSOARanges()` approach and support multiple CUs per OSO debug map entry, `SymbolFileDWARF::ResolveSymbolContext()` must be changed to test all candidate CUs. While these changes may be important and the code may really benefit from a cleanup, it goes far beyond the scope of this bug fix.

**The only reasonable alternative I see is to skip `AddOSOARanges()` for OSO debug map entries with multiple CUs.**



================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp:1531
+  if (m_compile_unit_infos.size() > 1)
+    return 0;
+
----------------
Skipping AddOSOARanges() here.


https://reviews.llvm.org/D52375