[clang-tidy] Fix a crash issue when clang-tidy runs with compilation database.

[hokein]

Summary

The clang-tidy will trigger an assertion if it's not in the building directory.

TEST:
cd <llvm-repo>/
./build/bin/clang-tidy --checks=-*,modernize-use-nullptr -p build tools/clang/tools/extra/clang-tidy/ClangTidy.cpp

The crash issue is gone after applying this patch.

Fixes PR24834, PR26241

[alexfh]

Thank you for working on this!

This change needs a lit test (ensure it fails/crashes without the patch and works fine after the patch).

clang-tidy/ClangTidyDiagnosticConsumer.cpp

343

Should this code handle possible errors from getCurrentWorkingDirectory()?

clang-tidy/ClangTidyDiagnosticConsumer.h

63

Please add a comment and explain also how this relates to ClangTidyMessage::FilePath and clang::tooling::Replacement::FilePath.

Additionally, describe possible values. For example, can this be an absolute path, a relative path or both? Can this be empty?

[hokein]

• Add lit test.
• Address review comments.

[hokein]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

343

Yeah, you are right. We need to handle the error here since in some cases like the source file is not exit, the SourceManager is unavailable.

[alexfh]

clang-tidy/ClangTidy.cpp

119

Maybe Change the directory to the one used during the analysis.?

clang-tidy/ClangTidyDiagnosticConsumer.cpp

123

This should be a StringRef for consistency.

clang-tidy/ClangTidyDiagnosticConsumer.h

68

compile_commands.json is one format of a compilation database, there are others. So it might be not very useful to name it here.

72

does not exist

test/clang-tidy/clang-tidy-run-with-database.cpp

2

I think, this test now needs // REQUIRES: shell

3

The test shouldn't change anything in the source directory. It can create any setup it needs in the temporary directory though (the %t placeholder would be useful for this).

[hokein]

• Address review comments.

[hokein]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

123

We might not use StringRef here since
SourceManager.getFileManager().getVirtualFileSystem().getCurrentWorkingDirectory() returns std::string.

clang-tidy/ClangTidyDiagnosticConsumer.h

68

However, I see the compile_commands.json is hardcoded in source. http://clang.llvm.org/doxygen/JSONCompilationDatabase_8cpp_source.html#l00111

[alexfh]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

123

As discussed offline, it makes sense to avoid calling SourceManager.getFileManager().getVirtualFileSystem().getCurrentWorkingDirectory() for each error, since it already has a cost of at least a string allocation/copy, but might also be an actual I/O operation. We can assume the working directory doesn't change while parsing a file, so we can cache it in the ClangTidyContext together with the current file name. Once we do this, we can get a StringRef to the working directory stored in ClangTidyContext and pass it here (but still store a std::string to avoid dependencies on a lifetime of an external string).

clang-tidy/ClangTidyDiagnosticConsumer.h

68

Yes, but this is just one implementation of a compilation database. There are other implementations as well.

68

nit: "the compilation database"

[hokein]

• Don't save BuildDirectory for every error in each check.

[hokein]

Some cleanup.

[hokein]

Remove an extra blank line.

[hokein]

Address one comment.

[hokein]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

123

Done. Now there is no need to add BuildDirectory in each ClangTidyError.

[alexfh]

In D17335#358613, @hokein wrote:

• Don't save BuildDirectory for every error in each check.

I think, it's not going to work with multiple files. There's just one Stats instance.

test/clang-tidy/clang-tidy-run-with-database.cpp

4

We need to test more stuff here, at least these come to mind:

1. -fix
2. multiple files in one clang-tidy invocation
3. files with the same name in different directories

[hokein]

Update:

• Update test to apply multiple files and fixes.
• Make it work with multiple files.

[hokein]

Make apply-fix work on relative path in command field of compilation database.

[alexfh]

clang-tidy/ClangTidyDiagnosticConsumer.h

76

As discussed offline, I'd like ClangTidyError to be independent of ClangTidyContext, unless this particular optimization saves significant resources.

[hokein]

Save the build directory in each error, making ClangTidyError not rely on ClangTidyContext.
The benchmark result shows that using StringRef doesn't improve performance greatly.

[hokein]

Don't modify unrelevant code.

[hokein]

Simplify test code.

[alexfh]

test/clang-tidy/clang-tidy-run-with-database.cpp

3

You don't need to create the root directory, since the mkdir -p calls below will take care of this.

16

I'm not sure how the rest of the test works. This grep -v call leaves everything except for the CHECK-lines in the %t.cpp, doesn't it?

17

Why don't you use %s here?

[hokein]

Address review comments.

[hokein]

test/clang-tidy/clang-tidy-run-with-database.cpp

17

Yeah, exactly.

[alexfh]

Looks good. Thank you for fixing this!

[hokein]

Nit: correct a comment.

clang-tidy/ClangTidy.h

 /// includes default check-specific options for all keys not overridden by \p
 /// Options.
 ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options);
 
 /// \brief Run a set of clang-tidy checks on a set of files.
 ///
 /// \param Profile if provided, it enables check profile collection in
 /// MatchFinder, and will contain the result of the profile.
-ClangTidyStats
-runClangTidy(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
+void
+runClangTidy(clang::tidy::ClangTidyContext& Context,
              const tooling::CompilationDatabase &Compilations,
-             ArrayRef<std::string> InputFiles,
-             std::vector<ClangTidyError> *Errors,
-             ProfileData *Profile = nullptr);
+             ArrayRef<std::string> InputFiles);
 
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
 //
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
 /// Errors containing fixes are automatically applied.
 void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
                   unsigned &WarningsAsErrorsCount);

clang-tidy/ClangTidy.cpp

ErrorReporter(bool ApplyFixes)
               DiagPrinter),
         SourceMgr(Diags, Files), Rewrite(SourceMgr, LangOpts),
         ApplyFixes(ApplyFixes), TotalFixes(0), AppliedFixes(0),
         WarningsAsErrors(0) {
     DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
     DiagPrinter->BeginSourceFile(LangOpts);
   }
 
+  SourceManager& getSourceManager() {
+    return SourceMgr;
+  }
+
   void reportDiagnostic(const ClangTidyError &Error) {
     const ClangTidyMessage &Message = Error.Message;
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
     // Contains a pair for each attempted fix: location and whether the fix was
     // applied successfully.
     SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
     {
       auto Level = static_cast<DiagnosticsEngine::Level>(Error.DiagLevel);
       std::string Name = Error.CheckName;
       if (Error.IsWarningAsError) {
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;
         WarningsAsErrors++;
       }

 ClangTidyASTConsumerFactory::CreateASTConsumer(
     clang::CompilerInstance &Compiler, StringRef File) {
   // FIXME: Move this to a separate method, so that CreateASTConsumer doesn't
   // modify Compiler.
   Context.setSourceManager(&Compiler.getSourceManager());
   Context.setCurrentFile(File);
   Context.setASTContext(&Compiler.getASTContext());
 
+  auto WorkingDir = Compiler.getSourceManager()
+                        .getFileManager()
+                        .getVirtualFileSystem()
+                        ->getCurrentWorkingDirectory();
+  if (WorkingDir) {
+    if (Context.getBuildDirectories().empty() ||
+        Context.getBuildDirectories().back() != WorkingDir.get())
+      Context.getBuildDirectories().push_back(WorkingDir.get());
+  }
+
   std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
   CheckFactories->createChecks(&Context, Checks);
 
   ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
   if (auto *P = Context.getCheckProfileData())
     FinderOptions.CheckProfiling.emplace(P->Records);
 
   std::unique_ptr<ast_matchers::MatchFinder> Finder(

 ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options) {
   clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options));
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
-ClangTidyStats
-runClangTidy(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
+void runClangTidy(clang::tidy::ClangTidyContext &Context,
                   const tooling::CompilationDatabase &Compilations,
-             ArrayRef<std::string> InputFiles,
-             std::vector<ClangTidyError> *Errors, ProfileData *Profile) {
+                  ArrayRef<std::string> InputFiles) {
   ClangTool Tool(Compilations, InputFiles);
-  clang::tidy::ClangTidyContext Context(std::move(OptionsProvider));
   ArgumentsAdjuster PerFileExtraArgumentsInserter = [&Context](
       const CommandLineArguments &Args, StringRef Filename) {
     ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
     CommandLineArguments AdjustedArgs;
     if (Opts.ExtraArgsBefore)
       AdjustedArgs = *Opts.ExtraArgsBefore;
     AdjustedArgs.insert(AdjustedArgs.begin(), Args.begin(), Args.end());
     if (Opts.ExtraArgs)
       AdjustedArgs.insert(AdjustedArgs.end(), Opts.ExtraArgs->begin(),
                           Opts.ExtraArgs->end());
     return AdjustedArgs;
   };
   Tool.appendArgumentsAdjuster(PerFileExtraArgumentsInserter);
-  if (Profile)
-    Context.setCheckProfileData(Profile);
 
   ClangTidyDiagnosticConsumer DiagConsumer(Context);
 
   Tool.setDiagnosticConsumer(&DiagConsumer);
 
   class ActionFactory : public FrontendActionFactory {
   public:
     ActionFactory(ClangTidyContext &Context) : ConsumerFactory(Context) {}
private:
       ClangTidyASTConsumerFactory *Factory;
     };
 
     ClangTidyASTConsumerFactory ConsumerFactory;
   };
 
   ActionFactory Factory(Context);
   Tool.run(&Factory);
-  *Errors = Context.getErrors();
-  return Context.getStats();
 }
 
 void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
                   unsigned &WarningsAsErrorsCount) {
   ErrorReporter Reporter(Fix);
-  for (const ClangTidyError &Error : Errors)
+  vfs::FileSystem &FileSystem =
+      *Reporter.getSourceManager().getFileManager().getVirtualFileSystem();
+  auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
+  if (!InitialWorkingDir)
+    llvm::report_fatal_error("Cannot get current working path.");
+
+  for (const ClangTidyError &Error : Errors) {
+    if (!Error.BuildDirectory.empty()) {
+      // By default, the working directory of file system is the current
+      // clang-tidy running directory.
+      //
+      // Change the directory to the one used during the analysis.
+      FileSystem.setCurrentWorkingDirectory(Error.BuildDirectory);
+    }
     Reporter.reportDiagnostic(Error);
+    // Return to the initial directory to correctly resolve next Error.
+    FileSystem.setCurrentWorkingDirectory(InitialWorkingDir.get());
+  }
   Reporter.Finish();
   WarningsAsErrorsCount += Reporter.getWarningsAsErrorsCount();
 }
 
 void exportReplacements(const std::vector<ClangTidyError> &Errors,
                         raw_ostream &OS) {
   tooling::TranslationUnitReplacements TUR;
   for (const ClangTidyError &Error : Errors)

clang-tidy/ClangTidyDiagnosticConsumer.h

 ///
 /// FIXME: Make Diagnostics flexible enough to support this directly.
 struct ClangTidyError {
   enum Level {
     Warning = DiagnosticsEngine::Warning,
     Error = DiagnosticsEngine::Error
   };
 
-  ClangTidyError(StringRef CheckName, Level DiagLevel, bool IsWarningAsError);
+  ClangTidyError(StringRef CheckName, Level DiagLevel, bool IsWarningAsError,
+                 StringRef BuildDirectory);
 
   std::string CheckName;
   ClangTidyMessage Message;
   tooling::Replacements Fix;
   SmallVector<ClangTidyMessage, 1> Notes;
 
+  // A build directory of the diagnostic source file. The string is owned by
+  // ClangTidyContext.
+  //
+  // It's an absolute path which is `directory` field of the source file in
+  // compilation database. If users don't specify the compilation database
+  // directory, it is the current directory where clang-tidy runs.
+  //
+  // Note: In unittest, it is empty.
+  StringRef BuildDirectory;
+
   Level DiagLevel;
   bool IsWarningAsError;
 };
 
 /// \brief Read-only set of strings represented as a list of positive and
 /// negative globs. Positive globs add all matched strings to the set, negative
 /// globs remove them in the order of appearance in the list.
 class GlobList {
public:
 
   /// \brief Set the output struct for profile data.
   ///
   /// Setting a non-null pointer here will enable profile collection in
   /// clang-tidy.
   void setCheckProfileData(ProfileData *Profile);
   ProfileData *getCheckProfileData() const { return Profile; }
 
+  /// \brief Returns all build directories.
+  std::vector<std::string> &getBuildDirectories() { return BuildDirectories; }
+
 private:
   // Calls setDiagnosticsEngine() and storeError().
   friend class ClangTidyDiagnosticConsumer;
 
   /// \brief Sets the \c DiagnosticsEngine so that Diagnostics can be generated
   /// correctly.
   void setDiagnosticsEngine(DiagnosticsEngine *Engine);
 
   /// \brief Store an \p Error.
   void storeError(const ClangTidyError &Error);
 
   std::vector<ClangTidyError> Errors;
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   std::unique_ptr<GlobList> CheckFilter;
   std::unique_ptr<GlobList> WarningAsErrorFilter;
 
   LangOptions LangOpts;
 
   ClangTidyStats Stats;
 
+  std::vector<std::string> BuildDirectories;
+
   llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
 
   ProfileData *Profile;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //

clang-tidy/ClangTidyDiagnosticConsumer.cpp

ClangTidyMessage::ClangTidyMessage(StringRef Message,
     : Message(Message) {
   assert(Loc.isValid() && Loc.isFileID());
   FilePath = Sources.getFilename(Loc);
   FileOffset = Sources.getFileOffset(Loc);
 }
 
 ClangTidyError::ClangTidyError(StringRef CheckName,
                                ClangTidyError::Level DiagLevel,
-                               bool IsWarningAsError)
-    : CheckName(CheckName), DiagLevel(DiagLevel),
+                               bool IsWarningAsError,
+                               StringRef BuildDirectory)
+    : CheckName(CheckName), BuildDirectory(BuildDirectory), DiagLevel(DiagLevel),
       IsWarningAsError(IsWarningAsError) {}
 
 // Returns true if GlobList starts with the negative indicator ('-'), removes it
 // from the GlobList.
 static bool ConsumeNegativeIndicator(StringRef &GlobList) {
   if (GlobList.startswith("-")) {
     GlobList = GlobList.substr(1);
     return true;
   }
   return false;
if (DiagLevel == DiagnosticsEngine::Error ||
       // code.
       Level = ClangTidyError::Error;
       LastErrorRelatesToUserCode = true;
       LastErrorPassesLineFilter = true;
     }
     bool IsWarningAsError =
         DiagLevel == DiagnosticsEngine::Warning &&
         Context.getWarningAsErrorFilter().contains(CheckName);
-    Errors.push_back(ClangTidyError(CheckName, Level, IsWarningAsError));
+    if (Context.getBuildDirectories().empty()) {
+      // In unittest, the BuildDirectories are empty.
+      Errors.push_back(
+          ClangTidyError(CheckName, Level, IsWarningAsError, StringRef()));
+    } else {
+      Errors.push_back(ClangTidyError(CheckName, Level, IsWarningAsError,
+                                      Context.getBuildDirectories().back()));
+    }
   }
 
   // FIXME: Provide correct LangOptions for each file.
   LangOptions LangOpts;
   ClangTidyDiagnosticRenderer Converter(
       LangOpts, &Context.DiagEngine->getDiagnosticOptions(), Errors.back());
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);

clang-tidy/tool/ClangTidyMain.cpp

static int clangTidyMain(int argc, const char **argv) {
   if (PathList.empty()) {
     llvm::errs() << "Error: no input files specified.\n";
     llvm::cl::PrintHelpMessage(/*Hidden=*/false, /*Categorized=*/true);
     return 1;
   }
 
   ProfileData Profile;
 
-  std::vector<ClangTidyError> Errors;
-  ClangTidyStats Stats =
-      runClangTidy(std::move(OptionsProvider), OptionsParser.getCompilations(),
-                   PathList, &Errors,
-                   EnableCheckProfile ? &Profile : nullptr);
+  clang::tidy::ClangTidyContext Context(std::move(OptionsProvider));
+  /// If Profile provided, it enables check profile collection in
+  /// MatchFinder, and will contain the result of the profile.
+  if (EnableCheckProfile)
+    Context.setCheckProfileData(&Profile);
+  runClangTidy(Context, OptionsParser.getCompilations(), PathList);
+  const std::vector<ClangTidyError> &Errors = Context.getErrors();
   bool FoundErrors =
       std::find_if(Errors.begin(), Errors.end(), [](const ClangTidyError &E) {
         return E.DiagLevel == ClangTidyError::Error;
       }) != Errors.end();
 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;
 
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
   handleErrors(Errors, (FixErrors || Fix) && !DisableFixes, WErrorCount);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
     std::error_code EC;
     llvm::raw_fd_ostream OS(ExportFixes, EC, llvm::sys::fs::F_None);
     if (EC) {
       llvm::errs() << "Error opening output file: " << EC.message() << '\n';
       return 1;
     }
     exportReplacements(Errors, OS);
   }
 
-  printStats(Stats);
+  printStats(Context.getStats());
   if (DisableFixes)
     llvm::errs()
         << "Found compiler errors, but -fix-errors was not specified.\n"
            "Fixes have NOT been applied.\n\n";
 
   if (EnableCheckProfile)
     printProfileData(Profile, llvm::errs());
 

test/clang-tidy/Inputs/compilation-database/template.json

+[
+{
+  "directory": "test_dir/a",
+  "command": "clang++ -o test.o test_dir/a/a.cpp",
+  "file": "test_dir/a/a.cpp"
+},
+{
+  "directory": "test_dir/a",
+  "command": "clang++ -o test.o test_dir/a/b.cpp",
+  "file": "test_dir/a/b.cpp"
+},
+{
+  "directory": "test_dir/",
+  "command": "clang++ -o test.o test_dir/b/b.cpp",
+  "file": "test_dir/b/b.cpp"
+},
+{
+  "directory": "test_dir/",
+  "command": "clang++ -o test.o test_dir/b/c.cpp",
+  "file": "test_dir/b/c.cpp"
+},
+{
+  "directory": "test_dir/",
+  "command": "clang++ -o test.o test_dir/b/not-exist.cpp",
+  "file": "test_dir/b/not-exist.cpp"
+}
+]

test/clang-tidy/clang-tidy-run-with-database.cpp

+// REQUIRES: shell
+// RUN: mkdir -p %T/compilation-database-test
+// RUN: mkdir -p %T/compilation-database-test/a
+// RUN: mkdir -p %T/compilation-database-test/b
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: sed 's|pointer|AA|g' %t.cpp > %T/compilation-database-test/a/a.cpp
+// RUN: sed 's|pointer|AA|g' %s > %T/compilation-database-test/a/a.cpp.msg
+// RUN: sed 's|pointer|AB|g' %t.cpp > %T/compilation-database-test/a/b.cpp
+// RUN: sed 's|pointer|AB|g' %s > %T/compilation-database-test/a/b.cpp.msg
+// RUN: sed 's|pointer|BB|g' %t.cpp > %T/compilation-database-test/b/b.cpp
+// RUN: sed 's|pointer|BB|g' %s > %T/compilation-database-test/b/b.cpp.msg
+// RUN: sed 's|pointer|BC|g' %t.cpp > %T/compilation-database-test/b/c.cpp
+// RUN: sed 's|pointer|BC|g' %s > %T/compilation-database-test/b/c.cpp.msg
+// RUN: sed 's|test_dir|%T/compilation-database-test|g' %S/Inputs/compilation-database/template.json > %T/compile_commands.json
+// RUN: clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/b/not-exist -header-filter=.*
+// RUN: clang-tidy --checks=-*,modernize-use-nullptr -p %T %T/compilation-database-test/a/a.cpp %T/compilation-database-test/a/b.cpp %T/compilation-database-test/b/b.cpp %T/compilation-database-test/b/c.cpp -header-filter=.* -fix
+// RUN: FileCheck -input-file=%T/compilation-database-test/a/a.cpp %T/compilation-database-test/a/a.cpp.msg -check-prefix=CHECK-FIX
+// RUN: FileCheck -input-file=%T/compilation-database-test/a/b.cpp %T/compilation-database-test/a/b.cpp.msg -check-prefix=CHECK-FIX
+// RUN: FileCheck -input-file=%T/compilation-database-test/b/b.cpp %T/compilation-database-test/b/b.cpp.msg -check-prefix=CHECK-FIX
+// RUN: FileCheck -input-file=%T/compilation-database-test/b/c.cpp %T/compilation-database-test/b/c.cpp.msg -check-prefix=CHECK-FIX
+
+#define NULL 0
+
+int *pointer = NULL;
+// CHECK-FIX: int *pointer = nullptr;
+// CHECK: warning: use nullptr [modernize-use-nullpt]