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

Tom Stellard via cfe-commits cfe-commits at lists.llvm.org
Fri Jun 3 20:20:26 PDT 2016


On Thu, Jun 02, 2016 at 01:27:51PM +0200, Alexander Kornienko wrote:
> Thank you, Tom. I'm not very familiar with the release process and it will
> indeed be better, if you merge the patch.
> 

Hi,

There were some merge conflicts, so I wasn't sure about merging the
patch. Can you either port the patch to the 3.8 branch and send it to me
or ask the maintainer to do the merge.

I'm still waiting on one more approval of another patch, so I'm going to delay -rc1
until Monday.

-Tom

> On Thu, Jun 2, 2016 at 3:27 AM, Tom Stellard <tom at stellard.net> wrote:
> 
> > On Wed, Jun 01, 2016 at 11:19:44PM +0200, Edoardo P. wrote:
> > > Ping.
> > >
> > > Deadline to merge the changes to the 3.8 branch is today.
> > >
> >
> > I will make sure to merge this before -rc1.  Thanks for reminding me.
> >
> > -Tom
> >
> > > Cheers,
> > > Edward-san
> > >
> > > 2016-05-24 20:25 GMT+02:00 Edoardo P. <ed0.88.prez at gmail.com>:
> > > > Ping,
> > > >
> > > > who's going to merge? I have no commit access.
> > > >
> > > > Cheers,
> > > > Edward-san
> > > >
> > > >
> > > > 2016-05-20 18:34 GMT+02:00 Tom Stellard <tom at stellard.net>:
> > > >> Hi,
> > > >>
> > > >> This looks fine to me, go ahead and merge.
> > > >>
> > > >> -Tom
> > > >>
> > > >> On Thu, May 19, 2016 at 08:29:14PM +0200, Alexander Kornienko wrote:
> > > >>> On Thu, May 19, 2016 at 4:45 PM, Hans Wennborg <hans at chromium.org>
> > wrote:
> > > >>>
> > > >>> > +Tom who manages 3.8.1
> > > >>> > +Alex who's owner of clang-tidy: is this ok for 3.8.1?
> > > >>> >
> > > >>>
> > > >>> Yes, would be nice to have this in 3.8.1. This fixes a rather
> > annoying
> > > >>> problem.
> > > >>>
> > > >>>
> > > >>> >
> > > >>> > On Thu, May 19, 2016 at 1:56 AM, Edoardo P. via cfe-commits
> > > >>> > <cfe-commits at lists.llvm.org> wrote:
> > > >>> > > Is it possible to port this commit to 3.8.1?
> > > >>> > >
> > > >>> > > Cheers,
> > > >>> > > Edward-san
> > > >>> > >
> > > >>> > > 2016-02-26 10:19 GMT+01:00 Haojian Wu via cfe-commits
> > > >>> > > <cfe-commits at lists.llvm.org>:
> > > >>> > >> Author: hokein
> > > >>> > >> Date: Fri Feb 26 03:19:33 2016
> > > >>> > >> New Revision: 261991
> > > >>> > >>
> > > >>> > >> URL: http://llvm.org/viewvc/llvm-project?rev=261991&view=rev
> > > >>> > >> Log:
> > > >>> > >> [clang-tidy] Fix a crash issue when clang-tidy runs with
> > compilation
> > > >>> > database.
> > > >>> > >>
> > > >>> > >> 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
> > > >>> > >>
> > > >>> > >> Reviewers: bkramer, alexfh
> > > >>> > >>
> > > >>> > >> Subscribers: rizsotto.mailinglist, cfe-commits
> > > >>> > >>
> > > >>> > >> Differential Revision: http://reviews.llvm.org/D17335
> > > >>> > >>
> > > >>> > >> Added:
> > > >>> > >>
> >  clang-tools-extra/trunk/test/clang-tidy/Inputs/compilation-database/
> > > >>> > >>
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/Inputs/compilation-database/template.json
> > > >>> > >>
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp
> > > >>> > >> Modified:
> > > >>> > >>     clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
> > > >>> > >>
> >  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
> > > >>> > >>
> >  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
> > > >>> > >>
> > > >>> > >> Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
> > > >>> > >> URL:
> > > >>> >
> > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp?rev=261991&r1=261990&r2=261991&view=diff
> > > >>> > >>
> > > >>> >
> > ==============================================================================
> > > >>> > >> --- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp (original)
> > > >>> > >> +++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp Fri Feb 26
> > > >>> > 03:19:33 2016
> > > >>> > >> @@ -107,6 +107,10 @@ public:
> > > >>> > >>      DiagPrinter->BeginSourceFile(LangOpts);
> > > >>> > >>    }
> > > >>> > >>
> > > >>> > >> +  SourceManager& getSourceManager() {
> > > >>> > >> +    return SourceMgr;
> > > >>> > >> +  }
> > > >>> > >> +
> > > >>> > >>    void reportDiagnostic(const ClangTidyError &Error) {
> > > >>> > >>      const ClangTidyMessage &Message = Error.Message;
> > > >>> > >>      SourceLocation Loc = getLocation(Message.FilePath,
> > > >>> > Message.FileOffset);
> > > >>> > >> @@ -124,7 +128,10 @@ public:
> > > >>> > >>        auto Diag = Diags.Report(Loc,
> > Diags.getCustomDiagID(Level, "%0
> > > >>> > [%1]"))
> > > >>> > >>                    << Message.Message << Name;
> > > >>> > >>        for (const tooling::Replacement &Fix : Error.Fix) {
> > > >>> > >> -        SourceLocation FixLoc = getLocation(Fix.getFilePath(),
> > > >>> > Fix.getOffset());
> > > >>> > >> +        SmallString<128> FixAbsoluteFilePath =
> > Fix.getFilePath();
> > > >>> > >> +        Files.makeAbsolutePath(FixAbsoluteFilePath);
> > > >>> > >> +        SourceLocation FixLoc =
> > > >>> > >> +            getLocation(FixAbsoluteFilePath, Fix.getOffset());
> > > >>> > >>          SourceLocation FixEndLoc =
> > > >>> > FixLoc.getLocWithOffset(Fix.getLength());
> > > >>> > >>          Diag <<
> > FixItHint::CreateReplacement(SourceRange(FixLoc,
> > > >>> > FixEndLoc),
> > > >>> > >>
> >  Fix.getReplacementText());
> > > >>> > >> @@ -232,6 +239,13 @@ ClangTidyASTConsumerFactory::CreateASTCo
> > > >>> > >>    Context.setCurrentFile(File);
> > > >>> > >>    Context.setASTContext(&Compiler.getASTContext());
> > > >>> > >>
> > > >>> > >> +  auto WorkingDir = Compiler.getSourceManager()
> > > >>> > >> +                        .getFileManager()
> > > >>> > >> +                        .getVirtualFileSystem()
> > > >>> > >> +                        ->getCurrentWorkingDirectory();
> > > >>> > >> +  if (WorkingDir)
> > > >>> > >> +    Context.setCurrentBuildDirectory(WorkingDir.get());
> > > >>> > >> +
> > > >>> > >>    std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
> > > >>> > >>    CheckFactories->createChecks(&Context, Checks);
> > > >>> > >>
> > > >>> > >> @@ -446,8 +460,24 @@ runClangTidy(std::unique_ptr<ClangTidyOp
> > > >>> > >>  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();
> > > >>> > >>  }
> > > >>> > >>
> > > >>> > >> Modified:
> > > >>> > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
> > > >>> > >> URL:
> > > >>> >
> > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=261991&r1=261990&r2=261991&view=diff
> > > >>> > >>
> > > >>> >
> > ==============================================================================
> > > >>> > >> ---
> > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
> > > >>> > (original)
> > > >>> > >> +++
> > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
> > > >>> > Fri Feb 26 03:19:33 2016
> > > >>> > >> @@ -116,8 +116,9 @@ ClangTidyMessage::ClangTidyMessage(Strin
> > > >>> > >>
> > > >>> > >>  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
> > > >>> > >> @@ -335,7 +336,8 @@ void ClangTidyDiagnosticConsumer::Handle
> > > >>> > >>      bool IsWarningAsError =
> > > >>> > >>          DiagLevel == DiagnosticsEngine::Warning &&
> > > >>> > >>          Context.getWarningAsErrorFilter().contains(CheckName);
> > > >>> > >> -    Errors.push_back(ClangTidyError(CheckName, Level,
> > > >>> > IsWarningAsError));
> > > >>> > >> +    Errors.push_back(ClangTidyError(CheckName, Level,
> > IsWarningAsError,
> > > >>> > >> +
> > > >>> > Context.getCurrentBuildDirectory()));
> > > >>> > >>    }
> > > >>> > >>
> > > >>> > >>    ClangTidyDiagnosticRenderer Converter(
> > > >>> > >>
> > > >>> > >> Modified:
> > > >>> > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
> > > >>> > >> URL:
> > > >>> >
> > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h?rev=261991&r1=261990&r2=261991&view=diff
> > > >>> > >>
> > > >>> >
> > ==============================================================================
> > > >>> > >> ---
> > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
> > > >>> > (original)
> > > >>> > >> +++
> > clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
> > > >>> > Fri Feb 26 03:19:33 2016
> > > >>> > >> @@ -57,13 +57,23 @@ struct ClangTidyError {
> > > >>> > >>      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.
> > > >>> > >> +  //
> > > >>> > >> +  // 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: it is empty in unittest.
> > > >>> > >> +  std::string BuildDirectory;
> > > >>> > >> +
> > > >>> > >>    Level DiagLevel;
> > > >>> > >>    bool IsWarningAsError;
> > > >>> > >>  };
> > > >>> > >> @@ -198,6 +208,16 @@ public:
> > > >>> > >>    void setCheckProfileData(ProfileData *Profile);
> > > >>> > >>    ProfileData *getCheckProfileData() const { return Profile; }
> > > >>> > >>
> > > >>> > >> +  /// \brief Should be called when starting to process new
> > translation
> > > >>> > unit.
> > > >>> > >> +  void setCurrentBuildDirectory(StringRef BuildDirectory) {
> > > >>> > >> +    CurrentBuildDirectory = BuildDirectory;
> > > >>> > >> +  }
> > > >>> > >> +
> > > >>> > >> +  /// \brief Returns build directory of the current
> > translation unit.
> > > >>> > >> +  const std::string &getCurrentBuildDirectory() {
> > > >>> > >> +    return CurrentBuildDirectory;
> > > >>> > >> +  }
> > > >>> > >> +
> > > >>> > >>  private:
> > > >>> > >>    // Calls setDiagnosticsEngine() and storeError().
> > > >>> > >>    friend class ClangTidyDiagnosticConsumer;
> > > >>> > >> @@ -222,6 +242,8 @@ private:
> > > >>> > >>
> > > >>> > >>    ClangTidyStats Stats;
> > > >>> > >>
> > > >>> > >> +  std::string CurrentBuildDirectory;
> > > >>> > >> +
> > > >>> > >>    llvm::DenseMap<unsigned, std::string>
> > CheckNamesByDiagnosticID;
> > > >>> > >>
> > > >>> > >>    ProfileData *Profile;
> > > >>> > >>
> > > >>> > >> Added:
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/Inputs/compilation-database/template.json
> > > >>> > >> URL:
> > > >>> >
> > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/Inputs/compilation-database/template.json?rev=261991&view=auto
> > > >>> > >>
> > > >>> >
> > ==============================================================================
> > > >>> > >> ---
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/Inputs/compilation-database/template.json
> > > >>> > (added)
> > > >>> > >> +++
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/Inputs/compilation-database/template.json
> > > >>> > Fri Feb 26 03:19:33 2016
> > > >>> > >> @@ -0,0 +1,32 @@
> > > >>> > >> +[
> > > >>> > >> +{
> > > >>> > >> +  "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/b",
> > > >>> > >> +  "command": "clang++ -o test.o ../b/c.cpp",
> > > >>> > >> +  "file": "test_dir/b/c.cpp"
> > > >>> > >> +},
> > > >>> > >> +{
> > > >>> > >> +  "directory": "test_dir/b",
> > > >>> > >> +  "command": "clang++ -I../include -o test.o ../b/d.cpp",
> > > >>> > >> +  "file": "test_dir/b/d.cpp"
> > > >>> > >> +},
> > > >>> > >> +{
> > > >>> > >> +  "directory": "test_dir/",
> > > >>> > >> +  "command": "clang++ -o test.o test_dir/b/not-exist.cpp",
> > > >>> > >> +  "file": "test_dir/b/not-exist.cpp"
> > > >>> > >> +}
> > > >>> > >> +]
> > > >>> > >>
> > > >>> > >> Added:
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp
> > > >>> > >> URL:
> > > >>> >
> > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp?rev=261991&view=auto
> > > >>> > >>
> > > >>> >
> > ==============================================================================
> > > >>> > >> ---
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp
> > > >>> > (added)
> > > >>> > >> +++
> > > >>> >
> > clang-tools-extra/trunk/test/clang-tidy/clang-tidy-run-with-database.cpp
> > > >>> > Fri Feb 26 03:19:33 2016
> > > >>> > >> @@ -0,0 +1,24 @@
> > > >>> > >> +// REQUIRES: shell
> > > >>> > >> +// RUN: mkdir -p %T/compilation-database-test/include
> > > >>> > >> +// RUN: mkdir -p %T/compilation-database-test/a
> > > >>> > >> +// RUN: mkdir -p %T/compilation-database-test/b
> > > >>> > >> +// RUN: echo 'int *AA = 0;' >
> > %T/compilation-database-test/a/a.cpp
> > > >>> > >> +// RUN: echo 'int *AB = 0;' >
> > %T/compilation-database-test/a/b.cpp
> > > >>> > >> +// RUN: echo 'int *BB = 0;' >
> > %T/compilation-database-test/b/b.cpp
> > > >>> > >> +// RUN: echo 'int *BC = 0;' >
> > %T/compilation-database-test/b/c.cpp
> > > >>> > >> +// RUN: echo 'int *HP = 0;' >
> > > >>> > %T/compilation-database-test/include/header.h
> > > >>> > >> +// RUN: echo '#include "header.h"' >
> > > >>> > %T/compilation-database-test/b/d.cpp
> > > >>> > >> +// 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
> > > >>> > %T/compilation-database-test/b/d.cpp -header-filter=.* -fix
> > > >>> > >> +// RUN: FileCheck
> > -input-file=%T/compilation-database-test/a/a.cpp %s
> > > >>> > -check-prefix=CHECK-FIX1
> > > >>> > >> +// RUN: FileCheck
> > -input-file=%T/compilation-database-test/a/b.cpp %s
> > > >>> > -check-prefix=CHECK-FIX2
> > > >>> > >> +// RUN: FileCheck
> > -input-file=%T/compilation-database-test/b/b.cpp %s
> > > >>> > -check-prefix=CHECK-FIX3
> > > >>> > >> +// RUN: FileCheck
> > -input-file=%T/compilation-database-test/b/c.cpp %s
> > > >>> > -check-prefix=CHECK-FIX4
> > > >>> > >> +// RUN: FileCheck
> > > >>> > -input-file=%T/compilation-database-test/include/header.h %s
> > > >>> > -check-prefix=CHECK-FIX5
> > > >>> > >> +
> > > >>> > >> +// CHECK-FIX1: int *AA = nullptr;
> > > >>> > >> +// CHECK-FIX2: int *AB = nullptr;
> > > >>> > >> +// CHECK-FIX3: int *BB = nullptr;
> > > >>> > >> +// CHECK-FIX4: int *BC = nullptr;
> > > >>> > >> +// CHECK-FIX5: int *HP = nullptr;
> > > >>> > >>
> > > >>> > >>
> > > >>> > >> _______________________________________________
> > > >>> > >> cfe-commits mailing list
> > > >>> > >> cfe-commits at lists.llvm.org
> > > >>> > >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> > > >>> > >
> > > >>> > >
> > > >>> > >
> > > >>> > > --
> > > >>> > > Mathematics is the language with which God has written the
> > universe.
> > > >>> > (Galilei)
> > > >>> > > _______________________________________________
> > > >>> > > cfe-commits mailing list
> > > >>> > > cfe-commits at lists.llvm.org
> > > >>> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> > > >>> >
> > > >
> > > >
> > > >
> > > > --
> > > > Mathematics is the language with which God has written the universe.
> > (Galilei)
> > >
> > >
> > >
> > > --
> > > Mathematics is the language with which God has written the universe.
> > (Galilei)
> >


More information about the cfe-commits mailing list