[Lldb-commits] [PATCH] D12658: Search variables based on clang::DeclContext and clang::Decl tree
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Thu Sep 10 15:51:05 PDT 2015
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
So a few themes I would like to get fixed:
- Don't parse everything in DWARF in case we ever need it (like parsing all namespaces and their contained variables, parse and cache all Block CompilerDeclContext within Block, etc), but do this lazily.
- Add functionality to the abstract CompilerDeclContext to be able to find things within it. If we have namespace "a" with 1000000 variables inside, we should only parse the variables we need and we should ask the CompilerDeclContext to find things for us instead of parsing all 1000000 variables and then finding "b" in that list of 1000000 variables. We should ask the CompilerDeclContext:
CompilerDecl var_decl = decl_ctx.FindVariable("a");
CompilerDeclList decl_list = decl_ctx.FindDecls("b");
- Parse things lazily as we need them and try not to store them in objects if we don't need to.
================
Comment at: include/lldb/Symbol/Block.h:362-366
@@ -360,1 +361,7 @@
+ CompilerDeclContext
+ GetClangDecl()
+ {
+ return m_clang_decl_context;
+ }
+
----------------
Why did you add this function? See the function just above it? Why did you not use that one? This should be removed and it should be lazily determined by Block::GetDeclContext(). The current implementation should be all you need.
================
Comment at: include/lldb/Symbol/Block.h:368-372
@@ +367,7 @@
+
+ void
+ SetClangDecl(CompilerDeclContext decl)
+ {
+ m_clang_decl_context = decl;
+ }
+
----------------
Remove this. Not needed. We should calculate the decl context in Block::GetDeclContext() as needed. No need to store this in the block permanently.
================
Comment at: include/lldb/Symbol/Block.h:492
@@ -478,2 +491,3 @@
m_parsed_child_blocks:1;
+ CompilerDeclContext m_clang_decl_context;
----------------
Remove this and let Block::GetDeclContext() create one for you each time. No need to permanently store this in a block instance.
================
Comment at: include/lldb/Symbol/ClangASTContext.h:1082-1170
@@ -1077,1 +1081,91 @@
+ clang::DeclContext *
+ CreateBlockDeclaration (clang::DeclContext *ctx, lldb_private::Block *block)
+ {
+ if (ctx != nullptr && block != nullptr)
+ {
+ clang::BlockDecl *decl = clang::BlockDecl::Create(*getASTContext(), ctx, clang::SourceLocation());
+ ctx->addDecl(decl);
+ CompilerDeclContext decl_context(this, llvm::dyn_cast<clang::DeclContext>(decl));
+ block->SetClangDecl(decl_context);
+ return decl;
+ }
+ return nullptr;
+ }
+
+ clang::UsingDirectiveDecl *
+ CreateUsingDirectiveDeclaration (clang::DeclContext *decl_ctx, clang::NamespaceDecl *ns_decl)
+ {
+ if (decl_ctx != nullptr && ns_decl != nullptr)
+ {
+ // TODO: run LCA between decl_tx and ns_decl
+ clang::UsingDirectiveDecl *using_decl = clang::UsingDirectiveDecl::Create(*getASTContext(),
+ decl_ctx,
+ clang::SourceLocation(),
+ clang::SourceLocation(),
+ clang::NestedNameSpecifierLoc(),
+ clang::SourceLocation(),
+ ns_decl,
+ GetTranslationUnitDecl(getASTContext()));
+ decl_ctx->addDecl(using_decl);
+ return using_decl;
+ }
+ return nullptr;
+ }
+
+ clang::UsingDecl *
+ CreateUsingDeclaration (clang::DeclContext *current_decl_ctx, clang::NamedDecl *target)
+ {
+ if (current_decl_ctx != nullptr && target != nullptr)
+ {
+ clang::UsingDecl *using_decl = clang::UsingDecl::Create(*getASTContext(),
+ current_decl_ctx,
+ clang::SourceLocation(),
+ clang::NestedNameSpecifierLoc(),
+ clang::DeclarationNameInfo(),
+ false);
+ clang::UsingShadowDecl *shadow_decl = clang::UsingShadowDecl::Create(*getASTContext(),
+ current_decl_ctx,
+ clang::SourceLocation(),
+ using_decl,
+ target);
+ using_decl->addShadowDecl(shadow_decl);
+ current_decl_ctx->addDecl(using_decl);
+ return using_decl;
+ }
+ return nullptr;
+ }
+
+ clang::VarDecl *
+ CreateVariableDeclaration (clang::DeclContext *decl_ctx, lldb::VariableSP var_sp, CompilerType clang_type)
+ {
+ if (decl_ctx != nullptr && var_sp && clang_type.IsValid())
+ {
+ const char *name = var_sp->GetUnqualifiedName().AsCString(nullptr);
+ clang::VarDecl *var_decl = clang::VarDecl::Create(*getASTContext(),
+ decl_ctx,
+ clang::SourceLocation(),
+ clang::SourceLocation(),
+ name && name[0] ? &getASTContext()->Idents.getOwn(name) : nullptr,
+ GetQualType(clang_type),
+ nullptr,
+ clang::SC_None);
+ var_decl->setAccess(clang::AS_public);
+ decl_ctx->addDecl(var_decl);
+ decl_ctx->makeDeclVisibleInContext(var_decl);
+ m_decl_to_var.insert(std::make_pair(var_decl, var_sp));
+ var_sp->SetVarDecl(CompilerDeclContext(this, var_decl));
+ return var_decl;
+ }
+ return nullptr;
+ }
+
+ lldb::VariableSP
+ GetVariableFromDecl (clang::VarDecl *var_decl)
+ {
+ auto decl_to_var_it = m_decl_to_var.find(var_decl);
+ if (decl_to_var_it != m_decl_to_var.end())
+ return decl_to_var_it->second;
+ return lldb::VariableSP();
+ }
+
----------------
Should probably move these to the .cpp file.
================
Comment at: include/lldb/Symbol/Variable.h:177-181
@@ +176,7 @@
+
+ CompilerDeclContext
+ GetVarDecl ()
+ {
+ return m_var_decl;
+ }
+
----------------
Is a variable a really a clang::DeclContext? This seems wrong. We might need a CompilerDecl class, but this seems wrong to put this in a CompilerDeclContext because a clang::VarDecl isn't a clang::DeclContext, it is a clang::Decl. Am I wrong?
================
Comment at: include/lldb/Symbol/Variable.h:183-187
@@ +182,7 @@
+
+ void
+ SetVarDecl (CompilerDeclContext var_decl)
+ {
+ m_var_decl = var_decl;
+ }
+
----------------
Can we get rid of this and lazily init it in GetVarDecl() with code like:
```
CompilerDeclContext
GetVarDecl ()
{
if (!m_var_decl)
{
SymbolFile *symfile = ...;
if (symfile)
m_var_decl = symfile->GetDeclContext();
}
return m_var_decl;
}
```
================
Comment at: source/Expression/ClangExpressionDeclMap.cpp:1358
@@ +1357,3 @@
+ CompilerDeclContext compiler_decl_context = sym_ctx.block != nullptr ? sym_ctx.block->GetClangDecl() : CompilerDeclContext();
+ ClangASTContext *ast_context = (ClangASTContext *)compiler_decl_context.GetTypeSystem();
+
----------------
Use llvm casting:
```
ClangASTContext *ast_context = llvm::dyn_cast_or_null<ClangASTContext>(compiler_decl_context.GetTypeSystem());
```
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1329-1339
@@ -1328,2 +1328,13 @@
+ CompilerDeclContext parent_decl_ctx;
+ if (block->GetParent() != nullptr && block->GetParent()->GetClangDecl().IsValid())
+ parent_decl_ctx = block->GetParent()->GetClangDecl();
+ else
+ parent_decl_ctx = block->GetDeclContext();
+
+ clang::DeclContext *containing_decl_context = ClangASTContext::DeclContextGetAsDeclContext(parent_decl_ctx);
+ if (containing_decl_context != nullptr)
+ GetClangASTContext().CreateBlockDeclaration(containing_decl_context, block);
+
+
++blocks_added;
----------------
Let the blocks parse these lazily as needed. We don't need to compute the CompilerDeclContext for any block unless we ask it to. The "Block::GetDeclContext()" call already exists and "does the right thing". No need for this.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3711-3713
@@ -3699,2 +3710,5 @@
}
+ if (DWARFDIE cu_die = dwarf_cu->GetDIE(dwarf_cu->GetFirstDIEOffset()))
+ for (auto d = cu_die.GetFirstChild(); d; d = d.GetSibling())
+ ParseImportedNamespace(sc, d, LLDB_INVALID_ADDRESS);
}
----------------
We shouldn't do this all the time. This will cause performance issues. If something needs a namespace, it should call through the existing SymbolFile virtual function:
```
lldb_private::CompilerDeclContext
FindNamespace (const lldb_private::SymbolContext& sc,
const lldb_private::ConstString &name,
const lldb_private::CompilerDeclContext *parent_decl_ctx) override;
```
If you need to lookup namespace "b" that is in namespace "b" (a::b) you would call the above function with: "a" as the name first and a CompilerDeclContext for the translation unit (we might need to add a function to lldb_private::CompileUnit:
```
class CompileUnit
{
CompilerDeclContext
GetDeclContext ();
}
```
And it will lazily compute its compiler decl context and return it:
```
CompilerDeclContext cu_decl_ctx = sc.comp_unit->GetDeclContext();
CompilerDeclContext a_decl_ctx = symfile->FindNamespace(sc, ConstString("a"), &cu_decl_ctx);
if (a_decl_ctx)
{
CompilerDeclContext b_decl_ctx = symfile->FindNamespace(sc, ConstString("b"), &a_decl_ctx);
}
```
We need to avoid adding code that parses all namespaces within a compile unit just because we might need the information later.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3721-3782
@@ -3706,1 +3720,64 @@
+void
+SymbolFileDWARF::ParseImportedNamespace
+(
+ const SymbolContext &sc,
+ const DWARFDIE die,
+ const lldb::addr_t func_low_pc
+)
+{
+ if (!die || (die.Tag() != DW_TAG_imported_module && die.Tag() != DW_TAG_imported_declaration))
+ return;
+
+ dw_offset_t imported_uid = die.GetAttributeValueAsReference(DW_AT_import, DW_INVALID_OFFSET);
+ if (UserIDMatches(imported_uid))
+ {
+ DWARFDebugInfo *debug_info = DebugInfo();
+ if (debug_info)
+ {
+ const DWARFDIE imported_die = debug_info->GetDIE(DIERef(imported_uid));
+ TypeSystem *type_system = GetTypeSystemForLanguage(die.GetCU()->GetLanguageType());
+ if (type_system)
+ {
+ DWARFASTParser *dwarf_ast = type_system->GetDWARFParser();
+ if (dwarf_ast != nullptr)
+ {
+ sc.comp_unit->GetVariableList(true);
+ clang::DeclContext *context = nullptr;
+ if (sc.block != nullptr)
+ context = ClangASTContext::DeclContextGetAsDeclContext(sc.block->GetClangDecl());
+ else
+ context = GetClangASTContext().GetTranslationUnitDecl(GetClangASTContext().getASTContext());
+
+ if (context != nullptr)
+ {
+ if (die.Tag() == DW_TAG_imported_module)
+ {
+ CompilerDeclContext ns_decl_context = dwarf_ast->GetDeclContextForUIDFromDWARF(imported_die);
+ if (clang::NamespaceDecl *ns_decl = ClangASTContext::DeclContextGetAsNamespaceDecl(ns_decl_context))
+ GetClangASTContext().CreateUsingDirectiveDeclaration(context, ns_decl);
+ }
+ else if (die.Tag() == DW_TAG_imported_declaration)
+ {
+ // TODO: handle imported declarations (using Namespace::Symbol) for other tags
+ if (imported_die.Tag() == DW_TAG_variable)
+ {
+ VariableSP var = ParseVariableDIE(sc, imported_die, func_low_pc);
+ if (var)
+ {
+ CompilerDeclContext clang_var_decl = var->GetVarDecl();
+ if (clang_var_decl)
+ {
+ clang::VarDecl *var_decl = (clang::VarDecl *)clang_var_decl.GetOpaqueDeclContext();
+ GetClangASTContext().CreateUsingDeclaration(context, var_decl);
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+ }
+}
+
----------------
So no clang:: code should exist in SymbolFileDWARF. Any code that uses "clang::" (which is a recent change) needs to be moved into DWARFASTParserClang for clang specific things.
I like to see abilities added to CompilerDeclContext that can discover the variable declarations inside a specific CompilerDeclContext.
Lets say we have:
```
namespace a
{
int aa;
}
```
We should be able to get a CompilerDeclContext for namespace "a" and then ask the "a_decl_ctx" to find a variable named "aa". This way we avoid having to parse all namespaces and all variables within that namespace (which could be a lot of variables). This means we would need to ask a CompilerDeclContext something like:
```
CompilerDeclContext a_decl_ctx = ...;
if (a_decl_ctx)
{
CompilerDecl var_decl = a_decl_ctx.FindVariable("aa");
```
Under the covers the CompilerDeclContext would need to call back into its symbol file (the TypeSystem inside the CompilerDeclContext has a link back to its SymbolFile).
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4159-4174
@@ -4071,2 +4158,18 @@
GetDIEToVariable()[die.GetDIE()] = var_sp;
+ if (spec_die)
+ GetDIEToVariable()[spec_die.GetDIE()] = var_sp;
+
+ if (var_sp && var_sp->GetType())
+ {
+ SymbolContext sc;
+ var_sp->CalculateSymbolContext(&sc);
+ CompilerDeclContext var_decl_context;
+ if (sc.block != nullptr)
+ var_decl_context = sc.block->GetClangDecl();
+ else
+ var_decl_context = var_sp->GetParentDeclContext();
+
+ if (var_decl_context)
+ GetClangASTContext().CreateVariableDeclaration(ClangASTContext::DeclContextGetAsDeclContext(var_decl_context), var_sp, var_sp->GetType()->GetForwardCompilerType());
+ }
}
----------------
I would like to avoid any of this kind of "parse a bunch of information 99% of the debugger will never use and store it permanently in case it ever gets used" kind of thing and discover variables through CompilerDeclContext as mentioned above.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4338-4339
@@ -4234,2 +4337,4 @@
}
+ else if (tag == DW_TAG_imported_module || tag == DW_TAG_imported_declaration)
+ ParseImportedNamespace(sc, die, func_low_pc);
}
----------------
I would like to avoid any of this kind of "parse a bunch of information 99% of the debugger will never use and store it permanently in case it ever gets used" kind of thing and discover variables through CompilerDeclContext as mentioned above.
http://reviews.llvm.org/D12658
More information about the lldb-commits
mailing list