[DWARFASTParserClang] Always complete types read from a module/PCH AST context.

[friss]

Summary

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.

[clayborg]

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?

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

181–182

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...

[friss]

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.

[friss]

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.

[friss]

Adding back a hunk I didn't mean to delete.

[clayborg]

Much better. Make sure Jim is ok with this as I am ok with it if he is.

[jingham]

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.

packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py

def test_specialized_typedef_from_pch(self):
             (testValue.GetError().GetCString()))
         self.assertTrue(
             memberValue.GetTypeName() == "int",
             "Member type incorrect")
         self.assertEqual(
             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")
+

packages/Python/lldbsuite/test/lang/cpp/gmodules/main.cpp

+class Foo::Bar { int i = 123; };
+
 int main(int argc, const char * argv[])
 {
     IntContainer test(42);
+    Foo::Bar bar;
     return 0; // break here
 }

packages/Python/lldbsuite/test/lang/cpp/gmodules/pch.h

 template<typename T>
 class GenericContainer {
   private:
     T storage;
 
   public:
     GenericContainer(T value) {
       storage = value;
     };
 };
 
 typedef GenericContainer<int> IntContainer;
+
+struct Foo {
+  class Bar;
+  Bar *bar;
+};

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

TypeSP DWARFASTParserClang::ParseTypeFromDWO(const DWARFDIE &die, Log *log) {
   TypeSP dwo_type_sp = dwo_types.GetTypeAtIndex(0);
   if (!dwo_type_sp)
     return TypeSP();
 
   lldb_private::CompilerType dwo_type = dwo_type_sp->GetForwardCompilerType();
 
   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(),
       NULL, LLDB_INVALID_UID, Type::eEncodingInvalid,
       &dwo_type_sp->GetDeclaration(), type, Type::eResolveStateForward));
 
   dwarf->GetTypeList()->Insert(type_sp);
   dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   clang::TagDecl *tag_decl = ClangASTContext::GetAsTagDecl(type);
   if (tag_decl)
     LinkDeclContextToDIE(tag_decl, die);
   else {
     clang::DeclContext *defn_decl_ctx = GetCachedClangDeclContextForDIE(die);
     if (defn_decl_ctx)
       LinkDeclContextToDIE(defn_decl_ctx, die);
   }
 
   return type_sp;
 }
 
+static void CompleteExternalTagDeclType(ClangASTImporter &ast_importer,
+                                        clang::DeclContext *decl_ctx,
+                                        DWARFDIE die,
+                                        const char *type_name_cstr) {
+  auto *tag_decl_ctx = clang::dyn_cast<clang::TagDecl>(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 &sc,
                                                const DWARFDIE &die, Log *log,
                                                bool *type_is_new_ptr) {
   TypeSP type_sp;
 
   if (type_is_new_ptr)
     *type_is_new_ptr = false;
 
if (type_ptr == NULL) {
         }
         assert(tag_decl_kind != -1);
         bool clang_type_was_created = false;
         clang_type.SetCompilerType(
             &m_ast, dwarf->GetForwardDeclDieToClangType().lookup(die.GetDIE()));
         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.
             const clang::Decl::Kind containing_decl_kind =
                 decl_ctx->getDeclKind();
             if (DeclKindIsCXXClass(containing_decl_kind))
               accessibility = default_accessibility;
           }