[Lldb-commits] [PATCH] D67022: Skip getting declarations for repeated DIEs (WIP)

Tue Sep 3 07:10:44 PDT 2019

clayborg added a comment.

In D67022#1653248 <https://reviews.llvm.org/D67022#1653248>, @guiandrade wrote:

> Hey guys,
>
> This change is more for me to get to know what you think. I've noticed that the GetDeclForUIDFromDWARF() calls inside SymbolFileDWARF::ParseDeclsForContext (https://github.com/llvm/llvm-project/blob/ef82098a800178a1f973abb8af86eaa690a29734/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L1216) often end up having no side effect besides generating a llvm::DenseMap::find invocation (https://github.com/llvm/llvm-project/blob/f07b4aff06d83c6ad25d95f456fbc12b2d2a0a0c/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L3322), especially when we evaluate multiple expressions inside the same scope. So I was wondering if there's a way to improve that. Do you guys think we could do something like what this change proposes, or am I missing something important? Thanks!

So this function gets called when clang wants to make sure that all decls inside of a function, class, struct, union are parsed. Parsed means DWARF has been converted into clang AST in the TypeSystem for the current DWARF file. This means if the DeclContext that is being used is something the clang expression parser wants to use and search, we can make sure all decls inside of a DeclContext are parsed so they will be visible. It will cause all DIEs to be parsed and all clang AST counterparts to be created for each DIE that gets returned. I really am not following what this patch does. The fact that you see:

  clang::Decl *DWARFASTParserClang::GetClangDeclForDIE(const DWARFDIE &die) 

seem like it is always just grabbing an entry from the cache (https://github.com/llvm/llvm-project/blob/f07b4aff06d83c6ad25d95f456fbc12b2d2a0a0c/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L3322) is only because this DWARFDIE has converted DWARF into a clang AST type already. The first time a decl gets parsed, it will fall through to the code below which converts the DWARF to clang AST types.

So, original code is doing what is intended. More comments below about how we can make this more efficient.

In D67022#1654754 <https://reviews.llvm.org/D67022#1654754>, @labath wrote:

> I am not very familiar with this code, but I don't see a reason why what you're doing could not work. However, it looks like the implementation of it could be done in a better way. For instance, this function seems to be the only caller of  `GetDIEForDeclContext`. So, we could replace it with something like ast_parser->ParseAllDiesForContext(decl_ctx). This would avoid materializing the std::vector, and the ast parser could store the list of processed dies in a more intelligent (memory friendly) fashion.

> The thing that's not clear to me is under what circumstances (if any) can a new DIE appear in a decl_context after ParseDeclsForContext has been called. @clayborg, do you have any idea?

No new DIEs should appear in a decl context in the future.

Speaking to efficiency we can do a few things:
1 - keep a cache of CompilerDeclContext's that have already been expanded. This way we can avoid the need to get a list of DIE, just to iterate through them and do nothing.

We can declare a new ivar in SymbolFileDWARF:

  std::set<CompilerDeclContext> m_parsed_decls_for_decl_ctx;

And then use this std::set. If the pair that is returned has "second" set to "false", then it has already been parsed, else it will set it and fall through and parse all decls one time:

  void SymbolFileDWARF::ParseDeclsForContext(CompilerDeclContext decl_ctx) {
    if (!m_parsed_decls_for_decl_ctx.insert(decl_ctx).second)
      return;
    // Fall through and do what we did before

2 - Change GetDIEForDeclContext to take a std::function or llvm::function that will get called with each DWARFDIE we need. This will avoid making a std::vector which we just iterate through. If we go this route we should rename this to ForEachDeclContextDIE(...).

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1212-1213
       ast_parser->GetDIEForDeclContext(decl_ctx);
+  DeclContextToOffsetKey offset_key =
+      std::make_pair(decl_ctx.GetOpaqueDeclContext(), type_system);
+  uint32_t &offset = m_decl_context_to_offset_map[offset_key];
----------------
delete these two lines

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1214
+      std::make_pair(decl_ctx.GetOpaqueDeclContext(), type_system);
+  uint32_t &offset = m_decl_context_to_offset_map[offset_key];

----------------
Why the reference to 'offset'? Also we shouldn't need to make a key, just use decl_ctx:
```
uint32_t offset = m_decl_context_to_offset_map[decl_ctx];
```
Also, m_decl_context_to_offset_map is not populated anywhere. Not sure what this will return as this was never assigned. It will probably just return a default constructed "uint32_t"??

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:493
+
+  typedef std::pair<void *, lldb_private::TypeSystem *> DeclContextToOffsetKey;
+  typedef std::map<DeclContextToOffsetKey, uint32_t> DeclContextToOffsetMap;
----------------
Any reason we are just using a lldb_private::CompilerDeclContext? Remove this line?

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:494
+  typedef std::pair<void *, lldb_private::TypeSystem *> DeclContextToOffsetKey;
+  typedef std::map<DeclContextToOffsetKey, uint32_t> DeclContextToOffsetMap;
+  DeclContextToOffsetMap m_decl_context_to_offset_map;
----------------
```
typedef std::map<CompilerDeclContext, uint32_t> DeclContextToOffsetMap;
```

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:495
+  typedef std::map<DeclContextToOffsetKey, uint32_t> DeclContextToOffsetMap;
+  DeclContextToOffsetMap m_decl_context_to_offset_map;
 };
----------------
I don't see 'm_decl_context_to_offset_map' being populated anywhere?

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67022/new/

https://reviews.llvm.org/D67022