[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327750: [DWARFASTParserClang] Complete external record types
before using them as a… (authored by friss, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D43592
Files:
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -205,6 +205,33 @@
return type_sp;
}
+static void CompleteExternalTagDeclType(ClangASTImporter _importer,
+clang::DeclContext *decl_ctx,
+DWARFDIE die,
+const char *type_name_cstr) {
+ auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
+ if (!tag_decl_ctx)
+return;
+
+ // If this type was not imported from an external AST, there's
+ // nothing to do.
+ CompilerType type = ClangASTContext::GetTypeForDecl(tag_decl_ctx);
+ if (!type || !ast_importer.CanImport(type))
+return;
+
+ auto qual_type = ClangUtil::GetQualType(type);
+ if (!ast_importer.RequireCompleteType(qual_type)) {
+die.GetDWARF()->GetObjectFile()->GetModule()->ReportError(
+"Unable to complete the Decl context for DIE '%s' at offset "
+"0x%8.8x.\nPlease file a bug report.",
+type_name_cstr ?: "", die.GetOffset());
+// We need to make the type look complete otherwise, we
+// might crash in Clang when adding children.
+if (ClangASTContext::StartTagDeclarationDefinition(type))
+ ClangASTContext::CompleteTagDeclarationDefinition(type);
+ }
+}
+
TypeSP DWARFASTParserClang::ParseTypeFromDWARF(const SymbolContext ,
const DWARFDIE , Log *log,
bool *type_is_new_ptr) {
@@ -795,6 +822,16 @@
if (!clang_type) {
clang::DeclContext *decl_ctx =
GetClangDeclContextContainingDIE(die, nullptr);
+
+ // If your decl context is a record that was imported from
+ // another AST context (in the gmodules case), we need to
+ // make sure the type backing the Decl is complete before
+ // adding children to it. This is not an issue in the
+ // non-gmodules case because the debug info will always contain
+ // a full definition of parent types in that case.
+ CompleteExternalTagDeclType(GetClangASTImporter(), decl_ctx, die,
+ type_name_cstr);
+
if (accessibility == eAccessNone && decl_ctx) {
// Check the decl context that contains this class/struct/union.
// If it is a class we must give it an accessibility.
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
@@ -10,3 +10,8 @@
};
typedef GenericContainer IntContainer;
+
+struct Foo {
+ class Bar;
+ Bar *bar;
+};
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
@@ -1,5 +1,8 @@
+class Foo::Bar { int i = 123; };
+
int main(int argc, const char * argv[])
{
IntContainer test(42);
+Foo::Bar bar;
return 0; // break here
}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
@@ -69,3 +69,26 @@
42,
memberValue.GetValueAsSigned(),
"Member value incorrect")
+
+testValue = frame.EvaluateExpression("bar")
+self.assertTrue(
+testValue.GetError().Success(),
+"Test expression value invalid: %s" %
+(testValue.GetError().GetCString()))
+self.assertTrue(
+testValue.GetTypeName() == "Foo::Bar",
+"Test expression type incorrect")
+
+
[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
jingham accepted this revision. jingham added a comment. I can't see a narrower way to do this. This sort of change makes me really wish we had "how many dies did you complete" metrics and some stress tests, so we can tell if we've just made some operation unreasonably more expensive and have to go back and really put our thinking caps on. But I think this should be fine. https://reviews.llvm.org/D43592 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
clayborg added a comment. Much better. Make sure Jim is ok with this as I am ok with it if he is. https://reviews.llvm.org/D43592 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
friss updated this revision to Diff 138733.
friss added a comment.
I experimented quite a bit with different approches to fix this bug
without always completing the types right after importing them from
the external AST.
This is the most principled approach I came up with. I applied the
new helper in only one place (the only one we have seen failing),
but we could potentially find more uses for it in the future.
https://reviews.llvm.org/D43592
Files:
packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
Index: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -179,12 +179,6 @@
lldb_private::CompilerType type =
GetClangASTImporter().CopyType(m_ast, dwo_type);
- // printf ("copied_qual_type: ast = %p, clang_type = %p, name =
- // '%s'\n", m_ast, copied_qual_type.getAsOpaquePtr(),
- // external_type->GetName().GetCString());
- if (!type)
-return TypeSP();
-
SymbolFileDWARF *dwarf = die.GetDWARF();
TypeSP type_sp(new Type(
die.GetID(), dwarf, dwo_type_sp->GetName(), dwo_type_sp->GetByteSize(),
@@ -205,6 +199,33 @@
return type_sp;
}
+static void CompleteExternalTagDeclType(ClangASTImporter _importer,
+clang::DeclContext *decl_ctx,
+DWARFDIE die,
+const char *type_name_cstr) {
+ auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
+ if (!tag_decl_ctx)
+return;
+
+ // If this type was not imported from an external AST, there's
+ // nothing to do.
+ CompilerType type = ClangASTContext::GetTypeForDecl(tag_decl_ctx);
+ if (!type || !ast_importer.CanImport(type))
+return;
+
+ auto qual_type = ClangUtil::GetQualType(type);
+ if (!ast_importer.RequireCompleteType(qual_type)) {
+die.GetDWARF()->GetObjectFile()->GetModule()->ReportError(
+"Unable to complete the Decl context for DIE '%s' at offset "
+"0x%8.8x.\nPlease file a bug report.",
+type_name_cstr ?: "", die.GetOffset());
+// We need to make the type look complete otherwise, we
+// might crash in Clang when adding children.
+if (ClangASTContext::StartTagDeclarationDefinition(type))
+ ClangASTContext::CompleteTagDeclarationDefinition(type);
+ }
+}
+
TypeSP DWARFASTParserClang::ParseTypeFromDWARF(const SymbolContext ,
const DWARFDIE , Log *log,
bool *type_is_new_ptr) {
@@ -795,6 +816,16 @@
if (!clang_type) {
clang::DeclContext *decl_ctx =
GetClangDeclContextContainingDIE(die, nullptr);
+
+ // If your decl context is a record that was imported from
+ // another AST context (in the gmodules case), we need to
+ // make sure the type backing the Decl is complete before
+ // adding children to it. This is not an issue in the
+ // non-gmodules case because the debug info will always contain
+ // a full definition of parent types in that case.
+ CompleteExternalTagDeclType(GetClangASTImporter(), decl_ctx, die,
+ type_name_cstr);
+
if (accessibility == eAccessNone && decl_ctx) {
// Check the decl context that contains this class/struct/union.
// If it is a class we must give it an accessibility.
Index: packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
@@ -10,3 +10,8 @@
};
typedef GenericContainer IntContainer;
+
+struct Foo {
+ class Bar;
+ Bar *bar;
+};
Index: packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
@@ -1,5 +1,8 @@
+class Foo::Bar { int i = 123; };
+
int main(int argc, const char * argv[])
{
IntContainer test(42);
+Foo::Bar bar;
return 0; // break here
}
Index: packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
@@ -69,3 +69,26 @@
42,
memberValue.GetValueAsSigned(),
"Member value incorrect")
+
+
[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
friss added a comment. Note that this code path is only triggered when importing debug info from a different AST context, it is not the common codepath. The issue in this case is that LLDB is crashing when using the incomplete Decl as the DeclContext for another one. I guess I could add calls to RequireCompleteType before every use of a DeclContext. But that would be costly too I think, mapping from a DeclContext to a CompilerType is not trivial. https://reviews.llvm.org/D43592 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Sean was the person with the most experience working on the expression parser and he wrote the clang AST importer, but Jim is now the expression parser expert. Jim: and ideas on how this really should be fixed? Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:182-184 + // The type we retrieve from the PCM debug info needs to be + // complete, otherwise we might crash when trying to extend it. + if (!type || !GetClangASTImporter().CompleteType(type)) We typically should never need to do the completion of a type manually like this. Many times we have variables that are like "foo *". For this variable, we might never need to complete the type since it is only a pointer. Furthermore, always completing a type will make LLDB very slow as every type we touch is now completing as soon as we import it. So while this patch might work, it will really affect LLDB performance. I think the right fix is probably elsewhere where someone isn't checking if the type is complete before doing the work that crashes clang... https://reviews.llvm.org/D43592 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D43592: [DWARFASTParserClang] Always complete types read from a module/PCH AST context.
friss created this revision.
friss added a reviewer: clayborg.
Herald added subscribers: JDevlieghere, aprantl.
The modified test would just crash without the code change. The reason is that
we would try to extend the Foo type imported from the PCH debug info when
adding the
Foo::Bar definitiion to it. This will crash if the type is not complete.
I pondered moving the CompleteType call inside of CopyType, but CopytType seems
to be used as a lower-level building block in other places so I decided not to.
ClangASTImporter is kinda scary. It has no comments and interacts with the Clang
ASTs which are not exactly easy to deal with. Any insight appreciated.
https://reviews.llvm.org/D43592
Files:
packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
Index: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -179,10 +179,9 @@
lldb_private::CompilerType type =
GetClangASTImporter().CopyType(m_ast, dwo_type);
- // printf ("copied_qual_type: ast = %p, clang_type = %p, name =
- // '%s'\n", m_ast, copied_qual_type.getAsOpaquePtr(),
- // external_type->GetName().GetCString());
- if (!type)
+ // The type we retrieve from the PCM debug info needs to be
+ // complete, otherwise we might crash when trying to extend it.
+ if (!type || !GetClangASTImporter().CompleteType(type))
return TypeSP();
SymbolFileDWARF *dwarf = die.GetDWARF();
Index: packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
@@ -10,3 +10,8 @@
};
typedef GenericContainer IntContainer;
+
+struct Foo {
+ class Bar;
+ Bar *bar;
+};
Index: packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp
@@ -1,5 +1,8 @@
+class Foo::Bar { int i = 123; };
+
int main(int argc, const char * argv[])
{
IntContainer test(42);
+Foo::Bar bar;
return 0; // break here
}
Index:
packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py
@@ -69,3 +69,26 @@
42,
memberValue.GetValueAsSigned(),
"Member value incorrect")
+
+testValue = frame.EvaluateExpression("bar")
+self.assertTrue(
+testValue.GetError().Success(),
+"Test expression value invalid: %s" %
+(testValue.GetError().GetCString()))
+self.assertTrue(
+testValue.GetTypeName() == "Foo::Bar",
+"Test expression type incorrect")
+
+memberValue = testValue.GetChildMemberWithName("i")
+self.assertTrue(
+memberValue.GetError().Success(),
+"Member value missing or invalid: %s" %
+(testValue.GetError().GetCString()))
+self.assertTrue(
+memberValue.GetTypeName() == "int",
+"Member type incorrect")
+self.assertEqual(
+123,
+memberValue.GetValueAsSigned(),
+"Member value incorrect")
+
Index: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -179,10 +179,9 @@
lldb_private::CompilerType type =
GetClangASTImporter().CopyType(m_ast, dwo_type);
- // printf ("copied_qual_type: ast = %p, clang_type = %p, name =
- // '%s'\n", m_ast, copied_qual_type.getAsOpaquePtr(),
- // external_type->GetName().GetCString());
- if (!type)
+ // The type we retrieve from the PCM debug info needs to be
+ // complete, otherwise we might crash when trying to extend it.
+ if (!type || !GetClangASTImporter().CompleteType(type))
return TypeSP();
SymbolFileDWARF *dwarf = die.GetDWARF();
Index: packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
===
--- packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
+++ packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h
@@ -10,3 +10,8 @@
};
typedef GenericContainer IntContainer;
+
+struct Foo {
+ class Bar;
+ Bar *bar;
+};
