Preserve CXX method overrides in ASTImporter

[lhames]

Summary

The ASTImporter should import CXX method overrides from the source context
when it imports a method decl.

[lhames]

To allow this to be tested I've added a flag to clang-import-test to dump the AST for the expression module, and modified clang's AST dumper code to print the list of overrides for each CXXMethodDecl. This should allow us to verify that override lists are preserved by AST imports.

The new dump format for CXXMethodDecl looks like:

| |-CXXMethodDecl 0x7fa9de878950 <line:6:3, col:24> col:8 used foo 'void (void)'
| | |-Overrides: [ 0x7fa9de89ce70 Base::foo 'void (void)' ]
| | |-CompoundStmt 0x7fa9de89ce38 <col:23, col:24>
| | `-OverrideAttr 0x7fa9de8789e8 <test.cpp:1:87>

It currently prints the full override list. Duncan and I had talked about printing only the "most recent" override, but that gets tricky where multiple inheritance is involved so I've opted for the simple scheme for now.

[spyffe]

Thanks for working on this! I have a few requests (some may arise from ignorance of capture semantics).

lib/AST/ASTDumper.cpp

1190

It looks like you only need OS for this. Would it make sense to explicitly capture that here?

1197

Is by-value the appropriate way to capture dumpOverride here? It seems like extra work.

lib/AST/ASTImporter.cpp

5510

This looks weird to me given that the only caller has ToMethod handy.
Rather than force another lookup, I'd just pass ToMethod in.

tools/clang-import-test/clang-import-test.cpp

257

I slightly prefer passing in DumpAST and letting the top level logic decide when to set it to true. I don't like setting a precedent of having Parse know whether it's handling the top-level expression or one of the dependencies, because it can make it harder to add features like indirection later.

327

Instead of true, pass DumpAST here (see above comment).

[lhames]

lib/AST/ASTDumper.cpp

1190

I stuck to the convention used in nearby code, but we could just use explicit capture of 'OS' or 'this'.

1197

dumpOverride only captures 'this' so it makes no practical difference whether we catch it by value or reference. For a larger lambda we'd certainly want to capture by reference.

lib/AST/ASTImporter.cpp

5510

Sure.

tools/clang-import-test/clang-import-test.cpp

257

Good call - will update this.

[lhames]

Updating based on Sean's comments.

[spyffe]

Looks good to me. Upon reflection, I think it's more important to adhere to the standard of the surrounding code in the ASTDumper, otherwise you're hurting reusability.

[lhames]

Committed in 305850, with a fix to handle destructor printing (which broke the Sema/ms_class_layout.cpp test case) in 305860.

lib/AST/ASTDumper.cpp

for (const ParmVarDecl *Parameter : D->parameters())
       dumpDecl(Parameter);
 
   if (const CXXConstructorDecl *C = dyn_cast<CXXConstructorDecl>(D))
     for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),
                                                  E = C->init_end();
          I != E; ++I)
       dumpCXXCtorInitializer(*I);
 
+  if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D))
+    if (MD->size_overridden_methods() != 0) {
+      auto dumpOverride =
+        [=](const CXXMethodDecl *D) {
+          SplitQualType T_split = D->getType().split();
+          OS << D << " " << D->getParent()->getName() << "::"
+             << D->getName() << " '"
+             << QualType::getAsString(T_split) << "'";
+        };
+
+      dumpChild([=] {
+        auto FirstOverrideItr = MD->begin_overridden_methods();
+        OS << "Overrides: [ ";
+        dumpOverride(*FirstOverrideItr);
+        for (const auto *Override :
+               llvm::make_range(FirstOverrideItr + 1,
+                                MD->end_overridden_methods()))
+          dumpOverride(Override);
+        OS << " ]";
+      });
+    }
+
   if (D->doesThisDeclarationHaveABody())
     dumpStmt(D->getBody());
 }
 
 void ASTDumper::VisitFieldDecl(const FieldDecl *D) {
   dumpName(D);
   dumpType(D->getType());
   if (D->isMutable())

lib/AST/ASTImporter.cpp

bool ImportContainerChecked(const InContainerTy &InContainer,
       return ImportArrayChecked(InContainer.begin(), InContainer.end(),
                                 OutContainer.begin());
     }
 
     template<typename InContainerTy, typename OIter>
     bool ImportArrayChecked(const InContainerTy &InContainer, OIter Obegin) {
       return ImportArrayChecked(InContainer.begin(), InContainer.end(), Obegin);
     }
+
+    // Importing overrides.
+    void ImportOverrides(CXXMethodDecl *ToMethod, CXXMethodDecl *FromMethod);
   };
 }
 
 //----------------------------------------------------------------------------
 // Import Types
 //----------------------------------------------------------------------------
 
 using namespace clang;
if (Stmt *FromBody = D->getBody()) {
     }
   }
 
   // FIXME: Other bits to merge?
 
   // Add this function to the lexical context.
   LexicalDC->addDeclInternal(ToFunction);
 
+  if (auto *FromCXXMethod = dyn_cast<CXXMethodDecl>(D))
+    ImportOverrides(cast<CXXMethodDecl>(ToFunction), FromCXXMethod);
+
   return ToFunction;
 }
 
 Decl *ASTNodeImporter::VisitCXXMethodDecl(CXXMethodDecl *D) {
   return VisitFunctionDecl(D);
 }
 
 Decl *ASTNodeImporter::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
Expr *ASTNodeImporter::VisitSubstNonTypeTemplateParmExpr(
   if (!Replacement)
     return nullptr;
 
   return new (Importer.getToContext()) SubstNonTypeTemplateParmExpr(
         T, E->getValueKind(), Importer.Import(E->getExprLoc()), Param,
         Replacement);
 }
 
+void ASTNodeImporter::ImportOverrides(CXXMethodDecl *ToMethod,
+                                      CXXMethodDecl *FromMethod) {
+  for (auto *FromOverriddenMethod : FromMethod->overridden_methods())
+    ToMethod->addOverriddenMethod(
+      cast<CXXMethodDecl>(Importer.Import(const_cast<CXXMethodDecl*>(
+                                            FromOverriddenMethod))));
+}
+
 ASTImporter::ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                          ASTContext &FromContext, FileManager &FromFileManager,
                          bool MinimalImport)
   : ToContext(ToContext), FromContext(FromContext),
     ToFileManager(ToFileManager), FromFileManager(FromFileManager),
     Minimal(MinimalImport), LastDiagFromFrom(false)
 {
   ImportedDecls[FromContext.getTranslationUnitDecl()]

tools/clang-import-test/clang-import-test.cpp

 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ExternalASTMerger.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/CodeGen/ModuleBuilder.h"
+#include "clang/Frontend/ASTConsumers.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/MultiplexConsumer.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Parse/ParseAST.h"
 
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
static llvm::cl::opt<bool>
     Direct("direct", llvm::cl::Optional,
              llvm::cl::desc("Use the parsed declarations without indirection"));
 
 static llvm::cl::list<std::string>
     ClangArgs("Xcc", llvm::cl::ZeroOrMore,
               llvm::cl::desc("Argument to pass to the CompilerInvocation"),
               llvm::cl::CommaSeparated);
 
+static llvm::cl::opt<bool>
+DumpAST("dump-ast", llvm::cl::init(false),
+        llvm::cl::desc("Dump combined AST"));
+
 namespace init_convenience {
 class TestDiagnosticConsumer : public DiagnosticConsumer {
 private:
   std::unique_ptr<TextDiagnosticBuffer> Passthrough;
   const LangOptions *LangOpts = nullptr;
 
 public:
   TestDiagnosticConsumer()
std::unique_ptr<CompilerInstance> BuildIndirect(std::unique_ptr<CompilerInstance> &CI) {
   std::unique_ptr<ASTContext> AST =
       init_convenience::BuildASTContext(*IndirectCI, *ST, *BC);
   IndirectCI->setASTContext(AST.release());
   AddExternalSource(*IndirectCI, CI);
   return IndirectCI;
 }
 
 llvm::Error ParseSource(const std::string &Path, CompilerInstance &CI,
-                        CodeGenerator &CG) {
+                        ASTConsumer &Consumer) {
   SourceManager &SM = CI.getSourceManager();
   const FileEntry *FE = CI.getFileManager().getFile(Path);
   if (!FE) {
     return llvm::make_error<llvm::StringError>(
         llvm::Twine("Couldn't open ", Path), std::error_code());
   }
   SM.setMainFileID(SM.createFileID(FE, SourceLocation(), SrcMgr::C_User));
-  ParseAST(CI.getPreprocessor(), &CG, CI.getASTContext());
+  ParseAST(CI.getPreprocessor(), &Consumer, CI.getASTContext());
   return llvm::Error::success();
 }
 
 llvm::Expected<std::unique_ptr<CompilerInstance>>
 Parse(const std::string &Path,
-      llvm::ArrayRef<std::unique_ptr<CompilerInstance>> Imports) {
+      llvm::ArrayRef<std::unique_ptr<CompilerInstance>> Imports,
+      bool ShouldDumpAST) {
   std::vector<const char *> ClangArgv(ClangArgs.size());
   std::transform(ClangArgs.begin(), ClangArgs.end(), ClangArgv.begin(),
                  [](const std::string &s) -> const char * { return s.data(); });
   std::unique_ptr<CompilerInstance> CI =
       init_convenience::BuildCompilerInstance(ClangArgv);
   auto ST = llvm::make_unique<SelectorTable>();
   auto BC = llvm::make_unique<Builtin::Context>();
   std::unique_ptr<ASTContext> AST =
       init_convenience::BuildASTContext(*CI, *ST, *BC);
   CI->setASTContext(AST.release());
   if (Imports.size())
     AddExternalSource(*CI, Imports);
 
+  std::vector<std::unique_ptr<ASTConsumer>> ASTConsumers;
+
   auto LLVMCtx = llvm::make_unique<llvm::LLVMContext>();
-  std::unique_ptr<CodeGenerator> CG =
-      init_convenience::BuildCodeGen(*CI, *LLVMCtx);
-  CG->Initialize(CI->getASTContext());
+  ASTConsumers.push_back(init_convenience::BuildCodeGen(*CI, *LLVMCtx));
+
+  if (ShouldDumpAST)
+    ASTConsumers.push_back(CreateASTDumper("", true, false, false));
 
   CI->getDiagnosticClient().BeginSourceFile(CI->getLangOpts(),
                                             &CI->getPreprocessor());
-  if (llvm::Error PE = ParseSource(Path, *CI, *CG)) {
+  MultiplexConsumer Consumers(std::move(ASTConsumers));
+  Consumers.Initialize(CI->getASTContext());
+
+  if (llvm::Error PE = ParseSource(Path, *CI, Consumers)) {
     return std::move(PE);
   }
   CI->getDiagnosticClient().EndSourceFile();
   if (CI->getDiagnosticClient().getNumErrors()) {
     return llvm::make_error<llvm::StringError>(
         "Errors occured while parsing the expression.", std::error_code());
   } else {
     return std::move(CI);
   }
 }
 
 } // end namespace
 
 int main(int argc, const char **argv) {
   const bool DisableCrashReporting = true;
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0], DisableCrashReporting);
   llvm::cl::ParseCommandLineOptions(argc, argv);
   std::vector<std::unique_ptr<CompilerInstance>> ImportCIs;
   for (auto I : Imports) {
-    llvm::Expected<std::unique_ptr<CompilerInstance>> ImportCI = Parse(I, {});
+    llvm::Expected<std::unique_ptr<CompilerInstance>> ImportCI =
+      Parse(I, {}, false);
     if (auto E = ImportCI.takeError()) {
       llvm::errs() << llvm::toString(std::move(E));
       exit(-1);
     } else {
       ImportCIs.push_back(std::move(*ImportCI));
     }
   }
   std::vector<std::unique_ptr<CompilerInstance>> IndirectCIs;
   if (!Direct) {
     for (auto &ImportCI : ImportCIs) {
       llvm::Expected<std::unique_ptr<CompilerInstance>> IndirectCI =
           BuildIndirect(ImportCI);
       if (auto E = IndirectCI.takeError()) {
         llvm::errs() << llvm::toString(std::move(E));
         exit(-1);
       } else {
         IndirectCIs.push_back(std::move(*IndirectCI));
       }
     }
   }
   llvm::Expected<std::unique_ptr<CompilerInstance>> ExpressionCI =
-      Parse(Expression, Direct ? ImportCIs : IndirectCIs);
+      Parse(Expression, Direct ? ImportCIs : IndirectCIs, DumpAST);
   if (auto E = ExpressionCI.takeError()) {
     llvm::errs() << llvm::toString(std::move(E));
     exit(-1);
   } else {
     return 0;
   }
 }