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

[Pavel Labath via lldb-commits]

On Mon, 14 May 2018 at 15:24, Jan Kratochvil via Phabricator <
reviews at reviews.llvm.org> wrote:

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

Ah, I see. Thanks for pointing that out (I'm fairly new to this part of the
code as well :)).

> 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'm not opposed to that, though I'm not sure if it makes things
substantially better. I'm not sure if this is a thing that can be
succinctly expressed in a name (DIEWhichCanBeUsedToAccessOtherDIEs). Maybe
just a comment explaining their difference would be enough.




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

Since it seems this is how things worked already, I don't think we need to
go out of our way here. If you can get an assert there then great.
Otherwise, I think the has_children should be enough.