On Mon, 14 May 2018 at 15:24, Jan Kratochvil via Phabricator < revi...@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. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
