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

[Pavel Labath via Phabricator via lldb-commits]

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?

> 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