[Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer

[Greg Clayton via lldb-commits]

> On May 14, 2018, at 6:36 AM, Pavel Labath via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> labath added a comment.
> 
> In https://reviews.llvm.org/D46810#1097740, @jankratochvil wrote:
> 
>> In https://reviews.llvm.org/D46810#1097503, @labath wrote:
>> 
>>> (If that is true, then I think this is workable, but there are still some details which need to be ironed out; e.g., `m_first_die.GetFirstChild()`)
>> 
>> 
>> The current code calling `GetUnitDIEOnly()` (returning `DWARFDIE`) or `GetUnitDIEPtrOnly` (returning `DWARFDebugInfoEntry *`) is never dealing with child DIEs of what it gets - it also makes sense as there is no guarantee they have been read in.
> 
> 
> I am not sure it's that simple. I've found at least one case (`SymbolFileDWARF::ParseImportedModuleswhere we call GetFirstChild() on the value returned by `GetUnitDIEOnly` (there may be others which are not easily greppable). Previously that would work if one would call this only after he'd know that all DIEs have been parsed. Now this will never work because GetFirstChild will return whatever is in the memory after `m_first_die`. I am not sure if this would be caught by asan straight away, though it will most likely cause a crash very soon.

> 
> I was thinking of making this safer by changing the `GetUnitDIEOnly` so that the caller has to explicitly request (either with an argument, or by splitting it into two functions) whether it wants a CU die, which can be used to access other DIEs, or just a bare attribute-only DIE. In second case, we would return &m_first_die, in the first case &m_die_array[0] (after making sure it's properly initialized). Then `m_first_die` can have `has_children` property set to false to enforce that noone accesses children that way.
> 
> WDYT?


One fix that would work it to make the DWARFDebugInfoEntry* inside the DWARFDie to be mutable and allow it to "fix" itself when a call is made. So the flow would be:

auto CUDie = cu->GetUnitDIEOnly();
for (auto child = CUDie.GetFirstChild(); ....

The call to DWARFDie::GetFirstChild() would replace "DWARFDebugInfoEntry *m_die" with the correct version by tracking if m_die is just the unit die only by making "DWARFDebugInfoEntry *m_die" into a llvm::PointerIntPair:

PointerIntPair< DWARFDebugInfoEntry *, 1> m_die;


We set the bit to 1 if m_die is just the unit DIE. Then we have an accessor on DWARFDie that can promote m_die to the full version if needed:

void DWARFDie::PromoteIfNeeded() {
  if (m_die.getInt()) {
    m_die.setPointer(m_cu->DIEPtr());
    m_die.setInt(false);
  }
}

> 
>> Thanks for checking validity of this patch, I was not sure whether LLDB code is intended to be thread-safe in the first place.
> 
> Yeah, thread safety is tricky. I think the DWARF parser was very single-threaded originally. Then, when we started building the index in a multi-threaded manner we needed to make a bunch of code thread-safe, which wasn't before. Now it looks like you are introducing even paralelization at an even deeper level (can't say I understand full what that means yet), so we'll need to make more code thread-safe.
> 
> 
> https://reviews.llvm.org/D46810
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180927/3e2cc159/attachment-0001.html>