[clang-extdef-mapping] Directly process .ast files

[thieta]

Summary

When doing CTU analysis setup you pre-compile .cpp to .ast and then
you run clang-extdef-mapping on the .cpp file as well. This is a
pretty slow process since we have to recompile the file each time.

With this patch you can now run clang-extdef-mapping directly on
the .ast file. That saves a lot of time.

I tried this on llvm/lib/AsmParser/Parser.cpp and running
extdef-mapping on the .cpp file took 5.4s on my machine.

While running it on the .ast file it took 2s.

This can save a lot of time for the setup phase of CTU analysis.

[thieta]

This still needs tests - but I wanted to get your early input on the approach here and what you all think.

[thieta]

Add test. Just repurpose the same test we already have but add a step to
generate ast first and then pushing that through extdef-mapping. It should
always produce the same result.

[thieta]

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

122

Not sure if I can just create a compilerinstance here and if I should feed it anymore data then I already do.

135

For some reason CI.getFileManager() often returns no filemanager - but sometimes it works? I just ended up creating my own to make the path absolute.

136

Pretty sure 128 is wrong here - but I searched the codebase and that seems to be the most common size used? I couldn't find anything using PATH_MAX or something like that.

141

I tried to figure out how to "run" the consumer on a already loaded ASTUnit - but I didn't find anything in the api for that and this way works since it's more or less what will happen when the consumer is executed anyway.

[thieta]

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

136

Never mind 128 is just the default size not the absolute size. Got it.

[thieta]

Clean-ups and error handling.

[martong]

Thanks, this is a great addition!

• Please use Capitalized names for all the variables (as per the LLVM coding standards suggests here).
• Could you please update the release notes part of Clang Static Analyzer as this is a cool user facing change.

I tried this on llvm/lib/AsmParser/Parser.cpp and running extdef-mapping on the .cpp file took 5.4s on my machine. While running it on the .ast file it took 2s.

I am wondering if you could have a comparison on more files? I think a measurement run on all clang source/ast files comparing the overall run-time could be even more persuasive.

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

122

I think it is okay to create the CI like you do. However, it would be enough to create that only once, not in every HandleAST call. Perhaps, it could be static or you might pass a reference to it from main.

124–129

Is there a way to reuse the DiagOpts (and the other Diag variables) over the multiple HandleAST calls? If not then we might use smart pointers to avoid leaking.

136

128 looks good to me.

142–143

Could this be a simple non-pointer variable?

151

I'd rather use a plural from here, there might be more than one source files.

152

Could you please add a one-liner explanatory comment for this loop?

165–167

Could you please extend the documentation of the tool with the new behavior?

[thieta]

Addressed review feedback.

[thieta]

Thanks for the review! I uploaded a new version addressing all (I think) of your feedback and added a release note.

The help text is a bit weird since it's using a commonoptionsparser we can't really change the text to add <ast0> as a specific input and we would have to create some kind of extension point in the parser or something. Not sure what you feel about this issue.

I will try to do some extensive testing - but I need to figure out a way to do all that ast building and extdef dumping in a automated way from the compile_commands database. I might run it on some internal code instead since I have a setup for that. Meanwhile let's make sure the code is in good shape.

Thanks!

[martong]

Thanks for the update! LGTM (with very minor naming nits).

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

146

160

171–172

[thieta]

Ran some internal benchmarks on 1570 files (C and C++ mixed but much more C++ than C):

Running clang-extdef-mapping on the source files took: 268s
Running clang-extdef-mapping on the AST: 102s

That's quite a large speed up if you already need to generate the AST files.

I'll fix the variable names and then merge this one unless someone complains.

[thieta]

Uppercase all variables

[steakhal]

Sorry about my delayed response. I was busy.
I've left a couple comments inline. Nothing serious.
Thanks for the patch!

clang/docs/ReleaseNotes.rst

593 ↗

(On Diff #442261)

I think we need to use double back ticks for preformatted/code texts.

595 ↗

(On Diff #442261)

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

42

Why is this not initialized in the initialized-list like the rest of the members?

149

What takes the ownership of CI? When is it deleted?

[thieta]

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

42

Ah no reason - I think I moved this around a few times so it just happened to end up here. I will push a NFC with that.

149

I don't think anyone takes ownership and it's never properly deleted. I don't think we really need to since this application just runs and exits and never really keep any state. Do you see a problem with it never being deleted?

[steakhal]

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

149

Not a big deal. I just think that in general, leak suggests bad software design.
On the other hand, one should not have non-primitive global variables, such as smart-pointers. I mean, it's still bad, but let's not touch it xD

clang/test/Analysis/func-mapping-test.cpp

 // RUN: %clang_extdef_map %s -- | FileCheck --implicit-check-not "c:@y" --implicit-check-not "c:@z" %s
+// RUN: %clang -emit-ast %s -o %t.ast
+// RUN: %clang_extdef_map %t.ast -- | FileCheck --implicit-check-not "c:@y" --implicit-check-not "c:@z" %s
 
 int f(int) {
   return 0;
 }
 // CHECK-DAG: 9:c:@F@f#I#
 
 extern const int x = 5;
 // CHECK-DAG: 4:c:@x

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

-//===- ClangExtDefMapGen.cpp -----------------------------------------------===//
+//===- ClangExtDefMapGen.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--------------------------------------------------------------------===//
 //
 // Clang tool which creates a list of defined functions and the files in which
 // they are defined.
 //
 //===--------------------------------------------------------------------===//
 
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Signals.h"
 #include <sstream>
 #include <string>
 
 using namespace llvm;
 using namespace clang;
 using namespace clang::cross_tu;
 using namespace clang::tooling;
 
-static cl::OptionCategory ClangExtDefMapGenCategory("clang-extdefmapgen options");
+static cl::OptionCategory
+    ClangExtDefMapGenCategory("clang-extdefmapgen options");
 
 class MapExtDefNamesConsumer : public ASTConsumer {
 public:
-  MapExtDefNamesConsumer(ASTContext &Context)
-      : Ctx(Context), SM(Context.getSourceManager()) {}
+  MapExtDefNamesConsumer(ASTContext &Context,
+                         StringRef astFilePath = StringRef())
+      : Ctx(Context), SM(Context.getSourceManager()) {
+    CurrentFileName = astFilePath.str();
+  }
 
   ~MapExtDefNamesConsumer() {
     // Flush results to standard output.
     llvm::outs() << createCrossTUIndexString(Index);
   }
 
   void HandleTranslationUnit(ASTContext &Context) override {
     handleDecl(Context.getTranslationUnitDecl());
protected:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  llvm::StringRef) override {
     return std::make_unique<MapExtDefNamesConsumer>(CI.getASTContext());
   }
 };
 
 static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
 
+void HandleAST(StringRef astPath) {
+
+  CompilerInstance CI;
+
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
+  TextDiagnosticPrinter *DiagClient =
+      new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
+      new DiagnosticsEngine(DiagID, &*DiagOpts, DiagClient));
+
+  std::unique_ptr<ASTUnit> unit = ASTUnit::LoadFromASTFile(
+      astPath.str(), CI.getPCHContainerOperations()->getRawReader(),
+      ASTUnit::LoadASTOnly, Diags, CI.getFileSystemOpts());
+
+  FileManager fm(CI.getFileSystemOpts());
+  SmallString<128> absPath(astPath);
+  fm.makeAbsolutePath(absPath);
+
+  std::unique_ptr<MapExtDefNamesConsumer> consumer =
+      std::make_unique<MapExtDefNamesConsumer>(unit->getASTContext(), absPath);
+  consumer->HandleTranslationUnit(unit->getASTContext());
+}
+
+void HandleFiles(ArrayRef<std::string> sourceFiles,
+                 CompilationDatabase &compilations) {
+  std::vector<std::string> sourceToBeParsed;
+  for (StringRef src : sourceFiles) {
+    if (src.endswith(".ast"))
+      HandleAST(src);
+    else
+      sourceToBeParsed.push_back(src.str());
+  }
+
+  if (!sourceToBeParsed.empty()) {
+    ClangTool Tool(compilations, sourceToBeParsed);
+    Tool.run(newFrontendActionFactory<MapExtDefNamesAction>().get());
+  }
+}
+
 int main(int argc, const char **argv) {
   // Print a stack trace if we signal out.
   sys::PrintStackTraceOnErrorSignal(argv[0], false);
   PrettyStackTraceProgram X(argc, argv);
 
   const char *Overview = "\nThis tool collects the USR name and location "
                          "of external definitions in the source files "
                          "(excluding headers).\n";
   auto ExpectedParser = CommonOptionsParser::create(
       argc, argv, ClangExtDefMapGenCategory, cl::ZeroOrMore, Overview);
   if (!ExpectedParser) {
     llvm::errs() << ExpectedParser.takeError();
     return 1;
   }
   CommonOptionsParser &OptionsParser = ExpectedParser.get();
 
-  ClangTool Tool(OptionsParser.getCompilations(),
-                 OptionsParser.getSourcePathList());
-
-  return Tool.run(newFrontendActionFactory<MapExtDefNamesAction>().get());
+  HandleFiles(OptionsParser.getSourcePathList(),
+              OptionsParser.getCompilations());
 }