> On May 14, 2018, at 6:36 AM, Pavel Labath via Phabricator
> <revi...@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
>
>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
