zturner created this revision.
zturner added reviewers: labath, clayborg, aleksandr.urakov, asmith, amccarth.
Herald added subscribers: JDevlieghere, aprantl.
`ParseDeclsForContext` was originally created to serve the very specific case
where the context is a function block. It was never intended to be used for
arbitrary DeclContexts, however due to the generic name, the DWARF and PDB
plugins implemented it in this way "just in case". Then, `lldb-test` came
along and decided to use it in that way.
Related to this, there are a set of functions in the `SymbolFile` class
interface whose requirements and expectations are not documented. For example,
if you call `ParseCompileUnitFunctions`, there's an inherent requirement that
you create entries in the underlying clang AST for these functions as well as
their signature types, because in order to create an `lldb_private::Function`
object, you have to pass it a `CompilerType` for the parameter representing the
signature.
On the other hand, there is no similar requirement (either inherent or
documented) if one were to call `ParseDeclsForContext`. Specifically, if one
calls `ParseDeclsForContext`, and some variable declarations, types, and other
things are added to the clang AST, is it necessary to create `lldb::Variable`,
`lldb::Type, etc objects representing them? Nobody knows. There is, however,
an //accidental// requirement, because since all of the plugins implemented
this just in case, `lldb-test` came along and used `ParsedDeclsForContext`, and
then wrote check lines that depended on this.
When I went to try and implemented the NativePDB reader, I did not adhere to
this (in fact, from a layering perspective I went out of my way to avoid it),
and as a result the existing DIA PDB tests don't work when the native PDB
reader is enabled, because they expect that calling ParseDeclsForContext will
modify the *module's* view of symbols, and not just the internal AST.
All of this confusion, however, can be avoided if we simply stick to using
`ParseDeclsForContext` for its original intended use case (blocks), and use a
different function (`ParseAllDebugSymbols`) for its intended use case which is,
unsuprisingly, to parse all the debug symbols (which is all `lldb-test` really
wanted to do anyway).
In the future, I would like to change `ParseDeclsForContext` to
`ParseDeclsForFunctionBlock`, then delete all of the dead code inside that
handles other types of DeclContexts (and probably even assert if the
DeclContext is anything other than a block).
A few PDB tests needed to be fixed up as a result of this, and this also
exposed a couple of bugs in the DIA PDB reader (doesn't matter much since it
should be going away soon, but worth mentioning) where the appropriate AST
entries weren't being created always.
https://reviews.llvm.org/D56418
Files:
lldb/lit/SymbolFile/PDB/enums-layout.test
lldb/lit/SymbolFile/PDB/type-quals.test
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
lldb/tools/lldb-test/lldb-test.cpp
Index: lldb/tools/lldb-test/lldb-test.cpp
===================================================================
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -518,6 +518,7 @@
Error opts::symbols::dumpAST(lldb_private::Module &Module) {
SymbolVendor &plugin = *Module.GetSymbolVendor();
+ Module.ParseAllDebugSymbols();
auto symfile = plugin.GetSymbolFile();
if (!symfile)
@@ -536,9 +537,6 @@
if (!tu)
return make_string_error("Can't retrieve translation unit declaration.");
- symfile->ParseDeclsForContext(CompilerDeclContext(
- clang_ast_ctx, static_cast<clang::DeclContext *>(tu)));
-
tu->print(outs());
return Error::success();
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
===================================================================
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
@@ -20,6 +20,8 @@
#include "llvm/DebugInfo/PDB/PDB.h"
#include "llvm/DebugInfo/PDB/PDBSymbolExe.h"
+class PDBASTParser;
+
class SymbolFilePDB : public lldb_private::SymbolFile {
public:
//------------------------------------------------------------------
@@ -224,6 +226,8 @@
void GetCompileUnitIndex(const llvm::pdb::PDBSymbolCompiland &pdb_compiland,
uint32_t &index);
+ PDBASTParser *GetPDBAstParser();
+
std::unique_ptr<llvm::pdb::PDBSymbolCompiland>
GetPDBCompilandByUID(uint32_t uid);
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===================================================================
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -314,6 +314,16 @@
func_type_uid, mangled, func_type, func_range);
sc.comp_unit->AddFunction(func_sp);
+
+ TypeSystem *type_system = GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
+ if (!type_system)
+ return nullptr;
+ ClangASTContext *clang_type_system =
+ llvm::dyn_cast_or_null<ClangASTContext>(type_system);
+ if (!clang_type_system)
+ return nullptr;
+ clang_type_system->GetPDBParser()->GetDeclForSymbol(pdb_func);
+
return func_sp.get();
}
@@ -1035,6 +1045,9 @@
if (variable_list)
variable_list->AddVariableIfUnique(var_sp);
++num_added;
+ PDBASTParser *ast = GetPDBAstParser();
+ if (ast)
+ ast->GetDeclForSymbol(*pdb_data);
}
}
}
@@ -1623,6 +1636,16 @@
return type_system;
}
+PDBASTParser *SymbolFilePDB::GetPDBAstParser() {
+ auto type_system = GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
+ auto clang_type_system = llvm::dyn_cast_or_null<ClangASTContext>(type_system);
+ if (!clang_type_system)
+ return nullptr;
+
+ return clang_type_system->GetPDBParser();
+}
+
+
lldb_private::CompilerDeclContext SymbolFilePDB::FindNamespace(
const lldb_private::SymbolContext &sc,
const lldb_private::ConstString &name,
Index: lldb/lit/SymbolFile/PDB/type-quals.test
===================================================================
--- lldb/lit/SymbolFile/PDB/type-quals.test
+++ lldb/lit/SymbolFile/PDB/type-quals.test
@@ -5,35 +5,35 @@
CHECK: Module [[MOD:.*]]
CHECK-DAG: {{^[0-9A-F]+}}: SymbolVendor ([[MOD]])
-CHECK: Type{{.*}} , name = "const int", size = 4, compiler_type = {{.*}} const int
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} const int *
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} const int **const
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} const int *const
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} const int *const *
-CHECK: Type{{.*}} , name = "Func1", {{.*}}, compiler_type = {{.*}} void (const int *, const int *, const int **const, const int *const *)
-
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} volatile int *
-CHECK: Type{{.*}} , name = "Func2", {{.*}}, compiler_type = {{.*}} void (volatile int *, volatile int *)
-
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} int *
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} int *&
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} int &
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} const int &
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} int &&
-CHECK: Type{{.*}} , name = "Func3", {{.*}}, compiler_type = {{.*}} void (int *&, int &, const int &, int &&)
+CHECK-DAG: Type{{.*}} , name = "const int", size = 4, compiler_type = {{.*}} const int
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} const int *
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} const int **const
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} const int *const
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} const int *const *
+CHECK-DAG: Type{{.*}} , name = "Func1", {{.*}}, compiler_type = {{.*}} void (const int *, const int *, const int **const, const int *const *)
+
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} volatile int *
+CHECK-DAG: Type{{.*}} , name = "Func2", {{.*}}, compiler_type = {{.*}} void (volatile int *, volatile int *)
+
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} int *
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} int *&
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} int &&
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} int &
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} const int &
+CHECK-DAG: Type{{.*}} , name = "Func3", {{.*}}, compiler_type = {{.*}} void (int *&, int &, const int &, int &&)
// FIXME: __unaligned is not supported.
-CHECK: Type{{.*}} , name = "Func4", {{.*}}, compiler_type = {{.*}} void (int *, int *)
+CHECK-DAG: Type{{.*}} , name = "Func4", {{.*}}, compiler_type = {{.*}} void (int *, int *)
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} int *__restrict
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} int &__restrict
-CHECK: Type{{.*}} , name = "Func5", {{.*}}, compiler_type = {{.*}} void (int, int *__restrict, int &__restrict)
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} int *__restrict
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} int &__restrict
+CHECK-DAG: Type{{.*}} , name = "Func5", {{.*}}, compiler_type = {{.*}} void (int, int *__restrict, int &__restrict)
-CHECK: Type{{.*}} , name = "Func6", {{.*}}, compiler_type = {{.*}} void (const volatile int *__restrict)
+CHECK-DAG: Type{{.*}} , name = "Func6", {{.*}}, compiler_type = {{.*}} void (const volatile int *__restrict)
-CHECK: Type{{.*}} , size = 400, compiler_type = {{.*}} volatile int *[100]
-CHECK: Type{{.*}} , size = 4000, compiler_type = {{.*}} volatile int *[10][100]
+CHECK-DAG: Type{{.*}} , size = 400, compiler_type = {{.*}} volatile int *[100]
+CHECK-DAG: Type{{.*}} , size = 4000, compiler_type = {{.*}} volatile int *[10][100]
-CHECK: Type{{.*}} , size = 4, compiler_type = {{.*}} long *__restrict
+CHECK-DAG: Type{{.*}} , size = 4, compiler_type = {{.*}} long *__restrict
CHECK-DAG: {{^[0-9A-F]+}}: CompileUnit{{[{]0x[0-9a-f]+[}]}}, language = "c++", file = '{{.*}}\TypeQualsTest.cpp'
Index: lldb/lit/SymbolFile/PDB/enums-layout.test
===================================================================
--- lldb/lit/SymbolFile/PDB/enums-layout.test
+++ lldb/lit/SymbolFile/PDB/enums-layout.test
@@ -1,44 +1,45 @@
REQUIRES: system-windows, msvc
RUN: %build --compiler=msvc --arch=32 --nodefaultlib --output=%T/SimpleTypesTest.cpp.enums.exe %S/Inputs/SimpleTypesTest.cpp
-RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck --check-prefix=ENUM %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck --check-prefix=ENUM_CONST %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck --check-prefix=ENUM_EMPTY %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck --check-prefix=ENUM_UCHAR %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck --check-prefix=ENUM_CLASS %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck --check-prefix=ENUM_STRUCT %s
; FIXME: PDB does not have information about scoped enumeration (Enum class) so the
; compiler type used is the same as the one for unscoped enumeration.
-CHECK: Module [[CU:.*]]
-CHECK-DAG: {{^[0-9A-F]+}}: SymbolVendor ([[CU]])
-CHECK: Type{{.*}} , name = "Enum", size = 4, decl = simpletypestest.cpp:19, compiler_type = {{.*}} enum Enum {
-CHECK-NEXT: RED,
-CHECK-NEXT: GREEN,
-CHECK-NEXT: BLUE
-CHECK-NEXT:}
+ENUM: Type{{.*}} , name = "Enum", size = 4, decl = simpletypestest.cpp:19, compiler_type = {{.*}} enum Enum {
+ENUM_NEXT: RED,
+ENUM_NEXT: GREEN,
+ENUM_NEXT: BLUE
+ENUM_NEXT:}
-CHECK: Type{{.*}} , name = "EnumConst", size = 4, decl = simpletypestest.cpp:22, compiler_type = {{.*}} enum EnumConst {
-CHECK-NEXT: LOW,
-CHECK-NEXT: NORMAL,
-CHECK-NEXT: HIGH
-CHECK-NEXT:}
+ENUM_CONST: Type{{.*}} , name = "EnumConst", size = 4, decl = simpletypestest.cpp:22, compiler_type = {{.*}} enum EnumConst {
+ENUM_CONST-NEXT: LOW,
+ENUM_CONST-NEXT: NORMAL,
+ENUM_CONST-NEXT: HIGH
+ENUM_CONST-NEXT:}
-CHECK: Type{{.*}} , name = "EnumEmpty", size = 4, decl = simpletypestest.cpp:25, compiler_type = {{.*}} enum EnumEmpty {
-CHECK-NEXT:}
+ENUM_EMPTY: Type{{.*}} , name = "EnumEmpty", size = 4, decl = simpletypestest.cpp:25, compiler_type = {{.*}} enum EnumEmpty {
+ENUM_EMPTY-NEXT:}
-CHECK: Type{{.*}} , name = "EnumUChar", size = 1, decl = simpletypestest.cpp:28, compiler_type = {{.*}} enum EnumUChar {
-CHECK-NEXT: ON,
-CHECK-NEXT: OFF,
-CHECK-NEXT: AUTO
-CHECK-NEXT:}
+ENUM_UCHAR: Type{{.*}} , name = "EnumUChar", size = 1, decl = simpletypestest.cpp:28, compiler_type = {{.*}} enum EnumUChar {
+ENUM_UCHAR-NEXT: ON,
+ENUM_UCHAR-NEXT: OFF,
+ENUM_UCHAR-NEXT: AUTO
+ENUM_UCHAR-NEXT:}
; Note that `enum EnumClass` is tested instead of `enum class EnumClass`
-CHECK: Type{{.*}} , name = "EnumClass", size = 4, decl = simpletypestest.cpp:32, compiler_type = {{.*}} enum EnumClass {
-CHECK-NEXT: YES,
-CHECK-NEXT: NO,
-CHECK-NEXT: DEFAULT
-CHECK-NEXT:}
-
-CHECK: Type{{.*}} , name = "EnumStruct", size = 4, decl = simpletypestest.cpp:35, compiler_type = {{.*}} enum EnumStruct {
-CHECK-NEXT: red,
-CHECK-NEXT: blue,
-CHECK-NEXT: black
-CHECK-NEXT:}
-
-CHECK-DAG: {{^[0-9A-F]+}}: CompileUnit{{[{]0x[0-9a-f]+[}]}}, language = "c++", file = '{{.*}}\SimpleTypesTest.cpp'
+ENUM_CLASS: Type{{.*}} , name = "EnumClass", size = 4, decl = simpletypestest.cpp:32, compiler_type = {{.*}} enum EnumClass {
+ENUM_CLASS-NEXT: YES,
+ENUM_CLASS-NEXT: NO,
+ENUM_CLASS-NEXT: DEFAULT
+ENUM_CLASS-NEXT:}
+
+ENUM_STRUCT: Type{{.*}} , name = "EnumStruct", size = 4, decl = simpletypestest.cpp:35, compiler_type = {{.*}} enum EnumStruct {
+ENUM_STRUCT-NEXT: red,
+ENUM_STRUCT-NEXT: blue,
+ENUM_STRUCT-NEXT: black
+ENUM_STRUCT-NEXT:}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
