[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

182–184

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) {
   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)
+  // 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();
   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));