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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to