This is an archive of the discontinued LLVM Phabricator instance.

Add -vfsoverlay option for Clang-Tidy
ClosedPublic

Authored by vladimir.plyashkun on Dec 21 2017, 11:09 PM.

Details

Summary

These changes introduce support for -vfsoverlay option in Clang-Tidy
The main reason for it:
In IDE integration we have two different pieces of source: editor and physical file.
File is placed in some project directory (let's call it "A")
We want to run clang-tidy exactly in this project directory "A", in order to get all .clang-tidy files in parent directories along with correct resolution for all relative include paths.
But in IDE, saving mechanism perfoms periodically.
So, to perform analysis of actual text on the fly, we need to save document on every modification (which is inefficient) or generate file in the temporary directory with actual text (it also leads to skipping .clang-tidy files and complicates include search paths resolution)
-vfsoverlay approach allows us to analyze source file in the project directory "A" but with actual data placed in another directory.

Diff Detail

Event Timeline

This comment was removed by vladimir.plyashkun.
ilya-biryukov added a subscriber: ilya-biryukov.EditedDec 22 2017, 9:18 AM

IIUC, you want to pass an overlay so that clang-tidy will read the file opened in the IDE from a different directory?

Could you add -ivfsoverlay option to your compile command instead?

$ clang --help | grep vfs
-ivfsoverlay <value>    Overlay the virtual filesystem described by file over the real file system

@ilya-biryukov
Yes, this is exactly what i need.
Unfortunately, -ivfsoverlay in the compile commands works for the compiler invocation, but it doesn't work for tooling.
E.g. this call:

clang-tidy -checks=* <file> -- -ivfsoverlay=<yaml_file>

has no effect.

FYI, i've create revision to support ivfsoverlay option in Tooling: https://reviews.llvm.org/D41594

Unfortunately, -ivfsoverlay in the compile commands works for the compiler invocation, but it doesn't work for tooling.

This looks like a bug in tooling, but let's wait for responses on the other therad.

It seems that clang-tidy will terribly broken with per-translation-unit overlays anyway. The problem is that clang-tidy seems to report errors and fixits in ErrorReporter class after running the tooling invocation, therefore it won't see any overlays that were local to each translation unit and may report wrong ranges, etc.
Probably global overlays (i.e. this patch) is probably the way to go.

clang-tidy/ClangTidy.cpp
528

Could we add a defaulted vfs::FileSystem BaseFS = getRealFileSystem() parameter to a constructor of ClangTool instead?
It seems like we're exposing implementation details of ClangTool here for no good reason.

Second part here (provided default virtual filesystem argument to ClangTool constructor): https://reviews.llvm.org/D41947

ilya-biryukov requested changes to this revision.Jan 17 2018, 9:07 AM

Sorry for the delay

clang-tidy/ClangTidy.cpp
92–94

We should pass BaseFS here as a parameter, similar to how we do that in ClangTool.
Piping it here from clangdTidyMain seems trivial.

93

DiagnosticsEngine& seems to be used to report errors in the source code, while the things we're seeing here are errors when initializing clang-tidy.
Could we move the code that creates vfs into ClangTidyMain.cpp and report errors directly into llvm::errs, similar to how we do that for other flags like list-checks, etc.

527

We should construct vfs before passing it to ClangTool and not modify it later.
I suggest creating BaseFS in clangTidyMain, see other comments too.

clang-tidy/ClangTidyOptions.h
114 ↗(On Diff #129440)

I think we should avoid putting VfsOverlay into ClangTidyOptions. Is there any reason why having a flag in ClangTidyMain.cpp is not enough?

This revision now requires changes to proceed.Jan 17 2018, 9:07 AM

Moved logic to ClangTidyMain

ilya-biryukov requested changes to this revision.Jan 18 2018, 7:58 AM
ilya-biryukov added inline comments.
clang-tidy/tool/ClangTidyMain.cpp
345

This code will print only the enum's integral value, we want to print an actual error message.
I don't think there's an easy way to reuse clang's diagnostics here, we should spell out the error message explicitly.

441

We should only create an overlay is -vfsoverlay was passed and use getRealFileSystem without wrappers in the common case.

444

Could we stop clang-tidy with an error if we couldn't create an overlay? That seems like a better option than silently running without an overlay when it was actually specified.

This revision now requires changes to proceed.Jan 18 2018, 7:58 AM

Fixed code review remarks.

Just a few more NITs and we're good to go

clang-tidy/tool/ClangTidyMain.cpp
348

We should probably print the error message (in addition to "not found", we could have permission errors, etc.)
Buffer.getError().message() would give a more informative message.

354

NIT: if (!FS) is equivalent, but less typing

439

NIT: remove braces around a single-statement return.

Some more nits were fixed.

This looks good, but we should add a test.
Should've noticed that before, sorry for the slowing this down a bit more. After the test is there, I'm happy to commit this change for you.

This looks good, but we should add a test.
Should've noticed that before, sorry for the slowing this down a bit more. After the test is there, I'm happy to commit this change for you.

IIUC, it will be little bit difficult to test it, because whole logic placed in the ClangTidyMain.
All existing clang-tidy unit tests use direct calls of ToolInvocation which is doesn't know about vfsoverlay options.

IIUC, it will be little bit difficult to test it, because whole logic placed in the ClangTidyMain.
All existing clang-tidy unit tests use direct calls of ToolInvocation which is doesn't know about vfsoverlay options.

We could test it with a lit test that runs clang-tidy directly and checks output using FileCheck. See clang-tools-extra/test/clang-tidy for clang-tidy tests using lit and clang/test/VFS for lit tests of vfs overlays in clang.

Thanks, now all should be fine!

ilya-biryukov accepted this revision.Jan 22 2018, 9:24 AM

Thanks for adding the test! LGTM, will submit it shortly.

This revision is now accepted and ready to land.Jan 22 2018, 9:24 AM
This revision was automatically updated to reflect the committed changes.