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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
