Page MenuHomePhabricator

[LibTooling] Make interface of VFS injection into ClangTool more user-friendly
Needs ReviewPublic

Authored by whisperity on Mar 30 2018, 8:35 AM.

Details

Summary

This patch extends upon D41947 because the interface that was landed from that patch isn't much user-friendly.

Injecting a custom virtual file system into the tool is a dangerous operation. If the real file system (where the installed Clang's headers reside) are not mirrored, it only turns out at ClangTool::run() that something was not mounted properly. Originally, the execution of a ClangTool always used the real filesystem.

Starting with D41947, the client code setting the tool up must manually create the OverlayFS (which is, in turn, added as an OverlayFS into the tool' inner OverlayFS) containing the real system and the user's intention. I believe this logic should not be duplicated in client code with calling pushOverlay and initialisation of FileSystem objects all the time.

Diff Detail

Event Timeline

whisperity created this revision.Mar 30 2018, 8:35 AM
alexfh edited reviewers, added: ilya-biryukov; removed: alexfh.Apr 4 2018, 7:19 AM

Thanks, this patch raises a very good point. Having a VFS that is not overlayed over RealFS is almost always the wrong thing to do.
On the other hand, I think it's useful to have the client code mention that it overlays over real filesystem, rather than relying on magic inside ClangTool.

I suggest we add a comment hinting that vfs you pass should be overlayed over the real filesystem, unless the client code knows what it's doing.
And provide an easy-to-use helper function to do that.

/// Creates overlayed file system with RealFS at the lowest priority and \p FS after that.
IntrusiveRefCntPtr<vfs::FileSystem> overlayRealFs(IntrusiveRefCntPtr<vfs::FileSystem> FS);

// .....

/// \param FileSystem ..... 
///     Don't pass a vfs that is not overlayed over the RealFS unless you know what you're doing. 
///     Use overlayRealFS helper to easily add the RealFS overlay.
include/clang/Tooling/Tooling.h
299

The convention in this file seems to be not to capitalize the terms, unless they are class names.
E.g. "ClangTool" is capitalized, but "clang tool" is not.
Maybe leave the comment as is?

whisperity updated this revision to Diff 141204.Apr 5 2018, 1:01 PM
  • Use an even more explicit way with the documentation requiring that the file system should be an overlay.
  • Add a method to easily overlay a FileSystem above the real one.
whisperity edited the summary of this revision. (Show Details)Apr 5 2018, 1:02 PM
whisperity updated this revision to Diff 141206.Apr 5 2018, 1:07 PM
whisperity marked an inline comment as done.

Simplify the patch.

ilya-biryukov added inline comments.Apr 9 2018, 5:10 AM
include/clang/Basic/VirtualFileSystem.h
313

I suggest leaving out the quotes around 'real'

lib/Basic/VirtualFileSystem.cpp
328

I generally agree that it might be useful, but given that we can't use dynamic_cast in LLVM code addressing this FIXME is probably not worth the effort.

And this patch is probably not the right place to add this comment, since it doesn't change OverlayFileSystem in any way.

unittests/Tooling/ToolingTest.cpp
411

Using '/' in paths would probably break on Windows.
Why do we need to add it in the first place?

435

It's not clear what this tests expects when looking at the code
A comment would be helpful.

Also consider matching on an actual error diagnostic (i.e. "<cstdio> not found", right?)

ilya-biryukov added inline comments.Apr 9 2018, 5:14 AM
include/clang/Basic/VirtualFileSystem.h
315

NIT: I'm not an expert in English, but shouldn't it be createOverlayOverReal.....
Also maybe shorten the suffix: createOverlayOverRealFS?

lib/Basic/VirtualFileSystem.cpp
372

Maybe add an assert the parameter is non-null?

whisperity added inline comments.Apr 9 2018, 5:45 AM
include/clang/Basic/VirtualFileSystem.h
315

According to this dictionary overlay can mean "to lay on " something. Although above could also work to eliminate saying "over" repeatedly.

unittests/Tooling/ToolingTest.cpp
411

Agreed, this will be removed. It was added because overlaying the real-FS changes the working directory in the memory filesystem too, which in the previous patch (where the overlay was done by the ClangTool constructor) broke where the file resides. It's not applicable anymore.

Although it's strange that saying make check-clang-tooling did NOT execute these tests and said everything passed, I had to run the build ToolingTest binary manually!

435

How can I match on the error message?

Pinging this as the talk has stalled.

klimek added inline comments.Jul 2 2018, 12:44 AM
lib/Basic/VirtualFileSystem.cpp
328

We can dyn_cast in LLVM code. It incurs some cost on the class hierarchy, but it's possible.

Softly pinging this. Perhaps we could discuss this and get it in before Clang7 rides off into the sunset?