[Lldb-commits] [PATCH] D73206: `DWARFASTParserClang::m_decl_ctx_to_die` `DWARFDIE`->`DIERef` for DWZ

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, clayborg.
jankratochvil added a project: LLDB.
Herald added a subscriber: aprantl.
Herald added a reviewer: shafik.

I think this patch is not good. I wrote it to as a solution to @labath's request that `DWARFDIE` should not increase in size from 16 bytes to 24 bytes (for a new 'main CU' pointer needed for DWZ).
I have found `DWARFDIE` is mostly used only for parameters and autovariables. I found (by `grep -r '<.*DWARF\(Base\|\)DIE' lldb`) only two cases of `DWARFDIE` in permanent structures:

- `DWARFASTParserClang::m_decl_ctx_to_die` refactored in this patch
- `ElaboratingDIEIterator::m_worklist` but I think it will never grow to any more `DWARFDIE`s than a few so it is worse to refactor it to `DIERef` which is slower and due to few entries it does not save any memory space.

I think the testcase improvement could be checked-in - after this patch the testcase was crashing trying to dereference original `(DWARFDebugInfoEntry *)1LL`.
As a replacement of `D70647` and as a solution to implement DWZ support (D40474 <https://reviews.llvm.org/D40474>) proposing multiple possibilities how to satisfy DWZ's need for `DWARFDIE::m_main_cu`, could you choose one?

1. `DWARFDIE` 16B->24B does not matter as `DWARFDIE` is not stored anywhere - maybe one could use this patch in such case.
2. PointerUnion for DWARFDIE::m_cu so that DWARFDIE remains 16B without DWZ.

  class DWARFBaseDIE {
    llvm::PointerUnion<DWARFUnit *, DWARFUnitPair *> m_cu;
    DWARFDebugInfoEntry *m_die;
  };
  class DWARFUnitPair {`
    DWARFUnit *m_cu;
    DWARFCompileUnit *m_main_cu;
  };

3. maybe: Compress `DWARFDIE::m_die` into 24-bit index of `DWARFDebugInfoEntry` in `DWARFUnit::m_die_array`. Chosen index instead of DIE offset as that should more easily fit into 24 bits even for largest CUs. The gained free 8 bits could be used for `DW_LANG_*` which is what MainCU is needed in DWZ. This assumes 32-bit LLDB host platform as they are still supported by LLDB. I would have to verify `DW_LANG_*` is all LLDB really needs from the MainCU, it is difficult to find it out from my current patchset.

Thanks for a feedback.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73206

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/unittests/Expression/DWARFExpressionTest.cpp
  lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
  lldb/unittests/TestingSupport/YAMLModuleTester.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73206.239585.patch
Type: text/x-patch
Size: 19894 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200122/a85b3fbd/attachment-0001.bin>