Differential D46810
3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer Authored by jankratochvil on May 13 2018, 12:14 PM.
Details + // GetUnitDIEPtrOnly() needs to return pointer to the first DIE. + // But the first element of m_die_array after ExtractDIEsIfNeeded(true) + // may move in memory after later ExtractDIEsIfNeeded(false). I haven't tried to reproduce it. DWARFDebugInfoEntry::collection m_die_array is std::vector, its data may move during its expansion.
Diff Detail
Event TimelineComment Actions I think I understand what is going on here, but I can't say for certain. Could you elaborate on the implementation details of this somewhere. It would be good to keep a note of this for future maintainers. My understanding of this is:
Is that an accurate description of what you are doing here? (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()) Comment Actions Yes. I will add a comment to the code.
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. Such code would already cause ASAN error accessing memory behind std::vector. So I find such assertion check orthogonal to this patch but I will prepare another patch for that. Thanks for checking validity of this patch, I was not sure whether LLDB code is intended to be thread-safe in the first place. Comment Actions 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?
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. Comment Actions OK, that is clearly a bug there, thanks for finding it. I see some new assertion check would be good (I may try even a compile time check by some new class wrapper). SymbolFileDWARF::ParseImportedModules should just call DIE(). DWARFDIE GetUnitDIEOnly() -> DWARFDebugInfoEntry *GetUnitDIEPtrOnly() -> ExtractDIEsIfNeeded(true ) -> CU DIE only DWARFDIE DIE () -> DWARFDebugInfoEntry * DIEPtr () -> ExtractDIEsIfNeeded(false) -> all DIEs
But there is already such function, it is called DIE(), we need no new parameter. Maybe we should rename those functions, their current names are cryptic, what about changing the second line to: DWARFDIE GetUnitDIEOnly() -> DWARFDebugInfoEntry *GetUnitDIEPtrOnly() -> ExtractDIEsIfNeeded(true ) -> CU DIE only DWARFDIE GetUnitAllDIEs() -> DWARFDebugInfoEntry *GetUnitAllDIEsPtr() -> ExtractDIEsIfNeeded(false) -> all DIEs
I would prefer an assert (or even a compilation error by some wrapper class) in such case but I agree this may be enough. Comment Actions So this problem exists both in the LLDB and LLVM DWARF parsers. I am not sure this fix is safe. I would rather fix this by fixing DWARFDIE class to "do the right thing". We should be able to teach the DWARFDIE class to replace its "m_die" with the updated "m_die" if a method ever causes DWARFDIE to need to expand all DIEs in a DWARFUnit. That seems like a much safer fix. Having m_first_die is not safe because it if you call DWARFDIE::GetFirstChild() it will just add 1 to the "m_die" and we will crash. All parent, sibling and child code just do pointer arithmetic to find their counterparts. So since DWARFDIE has the "DWARFUnit *m_cu;" and "DWARFDebugInfoEntry *m_die;" we should use DWARFDIE to abstract this from users. Anyone playing directly with DWARFDebugInfoEntry must know the rules and do the right thing or just use DWARFDIE. Comment Actions Trying to be smart while being lazy and multithreaded is going to make the code complicated (possibility for bugs) and/or introduce a lot of locking overhead. A lot simpler solution is to let the caller decide if it want's the full CU or just the root DIE, and then make it hard for him to get this wrong. This shouldn't be too much to ask, as that's pretty much what we have now, without the second part. The second part can be achieved by changing the GetUnitDIEOnly function to return a different type, say DWARFBasicDIE which does not even have child accessor functions. Then DWARFDIE can inherit from that and add the extra child/parent accessor methods. This way it will be impossible to get it wrong as you'll get a compile error if you try to access the children of a unit-only die. Comment Actions A better solution here would be to have two functions: one for parsing the Unit DIE only and one for parsing all DIEs: class DWARFUnit {
void ExtractUnitDIEIfNeeded();
size_t ExtractDIEsIfNeeded();
}Then the code becomes much simpler, we don't need the "m_die_array_size" function and logic is much cleaner, DWARFUnit::ExtractUnitDIEIfNeeded() will extract into m_first_die only and won't touch m_die_array at all. DWARFUnit::ExtractDIEsIfNeeded() will extract all DIEs into m_die_array. Then there is no need for worrying about m_die_array.size(). Comment Actions Statement isn't valid, but we should cleanup the DIE parsing code so we have dedicated parsing for the unit DIE only and for all DIEs and remove the m_die_array_size() function. Comment Actions +1 for the cleanup. The m_die_array_size function is just weird
Comment Actions Only question is if we remove the extra empty scope as noted in inline comments. Looks good.
Comment Actions @xbolva00 As you gave no reprodudcer it would be good to know how those two DWARFDebugInfoEntrys differ. Without that I can look at it more only next week - travelling.
| ||||||||||||||||||||||||||||||||||||||||||
@jankratochvil is this correct assert? Our downstream lldb based on lldb 7 with our custom targets hits this assert. If we remove it, we see no obvious breakages.