Page MenuHomePhabricator

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

Authored by hokein on Feb 17 2016, 7:24 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 48187.Feb 17 2016, 7:24 AM
hokein retitled this revision from to [clang-tidy] Fix a crash issue when clang-tidy runs with compilation database..
hokein updated this object.
hokein set the repository for this revision to rL LLVM.
hokein added a subscriber: cfe-commits.
alexfh edited edge metadata.Feb 17 2016, 7:41 AM

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 ↗(On Diff #48187)

Should this code handle possible errors from getCurrentWorkingDirectory()?

clang-tidy/ClangTidyDiagnosticConsumer.h
63 ↗(On Diff #48187)

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 updated this revision to Diff 48463.Feb 19 2016, 1:28 AM
  • Add lit test.
  • Address review comments.
hokein marked 2 inline comments as done.Feb 19 2016, 1:30 AM
hokein added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.cpp
343 ↗(On Diff #48463)

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 added inline comments.Feb 19 2016, 7:05 PM
clang-tidy/ClangTidy.cpp
115 ↗(On Diff #48463)

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

clang-tidy/ClangTidyDiagnosticConsumer.cpp
120 ↗(On Diff #48463)

This should be a StringRef for consistency.

clang-tidy/ClangTidyDiagnosticConsumer.h
66 ↗(On Diff #48463)

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

70 ↗(On Diff #48463)

does not exist

test/clang-tidy/clang-tidy-run-with-database.cpp
1 ↗(On Diff #48463)

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

2 ↗(On Diff #48463)

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 updated this revision to Diff 48657.Feb 22 2016, 1:51 AM
hokein marked an inline comment as done.
  • Address review comments.
hokein marked 4 inline comments as done.Feb 22 2016, 1:54 AM
hokein added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.cpp
120 ↗(On Diff #48657)

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

clang-tidy/ClangTidyDiagnosticConsumer.h
66 ↗(On Diff #48657)

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

alexfh added inline comments.Feb 22 2016, 5:00 AM
clang-tidy/ClangTidyDiagnosticConsumer.cpp
120 ↗(On Diff #48657)

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
66 ↗(On Diff #48657)

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

66 ↗(On Diff #48657)

nit: "the compilation database"

hokein updated this revision to Diff 48682.Feb 22 2016, 7:59 AM
hokein marked an inline comment as done.
  • Don't save BuildDirectory for every error in each check.
hokein updated this revision to Diff 48683.Feb 22 2016, 8:01 AM

Some cleanup.

hokein updated this revision to Diff 48684.Feb 22 2016, 8:03 AM

Remove an extra blank line.

hokein updated this revision to Diff 48686.Feb 22 2016, 8:07 AM
hokein marked an inline comment as done.

Address one comment.

hokein marked 2 inline comments as done.Feb 22 2016, 8:07 AM
hokein added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.cpp
122 ↗(On Diff #48684)

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

  • 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
3 ↗(On Diff #48684)

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
alexfh requested changes to this revision.Feb 24 2016, 5:44 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Feb 24 2016, 5:44 AM
hokein updated this revision to Diff 48924.Feb 24 2016, 6:29 AM
hokein edited edge metadata.

Update:

  • Update test to apply multiple files and fixes.
  • Make it work with multiple files.
hokein updated this revision to Diff 49027.Feb 25 2016, 3:06 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

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

alexfh requested changes to this revision.Feb 25 2016, 4:25 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.h
76 ↗(On Diff #49027)

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

This revision now requires changes to proceed.Feb 25 2016, 4:25 AM
hokein updated this revision to Diff 49042.Feb 25 2016, 4:55 AM
hokein edited edge metadata.

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 updated this revision to Diff 49044.Feb 25 2016, 4:58 AM
hokein edited edge metadata.

Don't modify unrelevant code.

hokein updated this revision to Diff 49054.Feb 25 2016, 6:51 AM

Simplify test code.

alexfh added inline comments.Feb 25 2016, 7:19 AM
test/clang-tidy/clang-tidy-run-with-database.cpp
2 ↗(On Diff #49054)

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

15 ↗(On Diff #49054)

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?

16 ↗(On Diff #49054)

Why don't you use %s here?

hokein updated this revision to Diff 49060.Feb 25 2016, 7:34 AM

Address review comments.

hokein marked 3 inline comments as done.Feb 25 2016, 7:35 AM
hokein added inline comments.
test/clang-tidy/clang-tidy-run-with-database.cpp
16 ↗(On Diff #49060)

Yeah, exactly.

alexfh accepted this revision.Feb 25 2016, 9:33 AM
alexfh edited edge metadata.

Looks good. Thank you for fixing this!

This revision is now accepted and ready to land.Feb 25 2016, 9:33 AM
hokein updated this revision to Diff 49159.Feb 26 2016, 1:03 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

Nit: correct a comment.

This revision was automatically updated to reflect the committed changes.