[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik created this revision.
shafik added reviewers: martong, balazske, teemperor, a_sidorin.
Herald added a subscriber: rnkovacs.
Herald added a reviewer: a.sidorin.
shafik added a comment.

I am open to different approaches to this problem, this is opening attempt at fixing it but I may be missing other interactions.

AFAICT starting the definition of `ToRecord` the way  `CompleteDecl(…)`  does when we know `FromRecord` is not defined is just broken for the `RecordDecl` case if we have bases.

It took me a lot of time to come up with this reproducer from a real life issue debugging `llc`.


In `ImportContext(…)` we may call into `CompleteDecl(…)` which if `FromRecrord` is not defined will start the definition of a `ToRecord` but from what I can tell at least one of the paths though here don't ensure we complete the definition. 
For a `RecordDecl` this can be problematic since this means we won’t import base classes and we won’t have any of the methods or types we inherit from these bases.

One such path is through `VisitTypedefNameDecl(…)` which is exercised by the reproducer.

For LLDB expression parsing this results in expressions failing but sometimes succeeding in subsequent attempts e.g.:

  (lldb) p BB->dump()
  error: no member named 'dump' in 'B'
  (lldb) p BB->dump()
  (lldb) 

This happens because the first time through `FromRecord` is not defined but in subsequent attempts through it may be.


https://reviews.llvm.org/D78000

Files:
  clang/lib/AST/ASTImporter.cpp
  lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
  lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp


Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,36 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+    return true; //%self.expect_expr("DD->dump()", result_type="int",
+                 // result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -8166,7 +8166,19 @@
           FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
         return std::move(Err);
     } else {
-      CompleteDecl(ToRecord);
+      // If FromRecord is not defined we need to force it to be.
+      // Simply calling CompleteDecl(...) for a RecordDecl will break some cases
+      // it will start the definition but we never finish it.
+      // If a RecordDecl has base classes they won't be imported and we will
+      // be missing anything that we inherit from those bases.
+      if (FromRecord->hasExternalLexicalStorage() &&
+          !FromRecord->isCompleteDefinition())
+        FromRecord->getASTContext().getExternalSource()->CompleteType(
+            FromRecord);
+
+      if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+              FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+        return std::move(Err);
     }
   } else if (auto *ToEnum = dyn_cast<EnumDecl>(ToDC)) {
     auto *FromEnum = cast<EnumDecl>(FromDC);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78000.256920.patch
Type: text/x-patch
Size: 2816 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200413/bb3d8bc8/attachment.bin>