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

Pavel Labath via lldb-commits lldb-commits at lists.llvm.org
Mon May 14 07:37:36 PDT 2018

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

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.

More information about the lldb-commits mailing list