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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In https://reviews.llvm.org/D46810#1097829, @labath wrote:

> 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`


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



> 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).

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



> Then `m_first_die` can have `has_children` property set to false to enforce that noone accesses children that way.

I would prefer an assert (or even a compilation error by some wrapper class) in such case but I agree this may be enough.


https://reviews.llvm.org/D46810