This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory
ClosedPublic

Authored by kadircet on Jun 16 2020, 3:18 AM.

Details

Summary

We've faced a couple of problems when the returned FS didn't have the
proper working directory. New signature makes the API safer against such
problems.

Diff Detail

Event Timeline

kadircet created this revision.Jun 16 2020, 3:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 3:18 AM

Looks like an improvement but I don't like the (".") in various places.

Maybe make the param optional<StringRef> and don't cd if it's none?
I wouldn't give it a default arg though, the idea is to force a choice.

clang-tools-extra/clangd/ClangdServer.cpp
187

Not sure what to do in this patch, but ultimately I don't think we must/should pass an FS of any form into this callback.
(Doesn't make sense to assume getting options needs the FS, embedders can use the context to know what version of the FS to use if needed)

kadircet updated this revision to Diff 271312.Jun 17 2020, 2:46 AM
kadircet marked 2 inline comments as done.
  • Change signature to llvm::Optional<StringRef> to accomodate call sites that don't want to cd
kadircet updated this revision to Diff 271328.Jun 17 2020, 4:08 AM
  • Provide two overloads to make implicit string -> StringRef conversion possible.
kadircet added inline comments.Jun 17 2020, 4:17 AM
clang-tools-extra/clangd/ClangdServer.cpp
187

sounds good, will delete in a follow-up patch.

sammccall accepted this revision.Jun 17 2020, 5:22 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
322

nit: I think we should have /*CWD=*/ on the None (and elsewhere)

400

seems weird that we provide a working directory sometimes but not other times when getting format style.
I think this is because not providing one is suspicious, it's not actually needed, but sometimes we have one around?

Maybe just make getFormatStyleForFile take a FSProvider instead?

551

we could plumb in threadsafefs here too...

clang-tools-extra/clangd/Preamble.cpp
426

I don't see a corresponding removed CD - is this a bugfix?

clang-tools-extra/clangd/support/FSProvider.cpp
81

Nit: RealFileSystemProvider isn't the current class.

What about elog("VFS: failed to set CWD to {0}: {1}", CWD, Err.message())?

clang-tools-extra/clangd/support/FSProvider.h
33–39

/// Initial working directory is arbitrary.

35

"Similar to above one" seems a bit awkward, maybe "As above"?

curret -> current

39

did you want this to be virtual so that a truly virtual FS e.g. where the working directory is just a variable can be constructed with it directly?

This revision is now accepted and ready to land.Jun 17 2020, 5:22 AM
kadircet marked 9 inline comments as done.Jun 17 2020, 6:37 AM
kadircet added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
551

this function is also used by tweaks (DefineOutline), and they don't have access to FSProvider. I would rather do that plumbing on a separate patch.

clang-tools-extra/clangd/Preamble.cpp
426

not really, CWD doesn't matter for preamble scanning as we only go over current file contents that are set explicitly.

It will actually be dropped by https://reviews.llvm.org/D81719, changing it to pass None here to not confuse.

kadircet updated this revision to Diff 271354.Jun 17 2020, 6:37 AM
kadircet marked an inline comment as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.
uabelho added inline comments.
clang-tools-extra/clangd/support/FSProvider.h
39

I noticed that gcc (7.4) warns on this line:

/data/repo/master/clang-tools-extra/clangd/support/FSProvider.h:39:3: warning: 'virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> clang::clangd::FileSystemProvider::getFileSystem(clang::clangd::PathRef) const' was hidden [-Woverloaded-virtual]
   getFileSystem(PathRef CWD) const;
   ^~~~~~~~~~~~~
45

Similar warning here as well:

/data/repo/master/clang-tools-extra/clangd/support/FSProvider.h:45:7: warning:   by 'virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> clang::clangd::RealFileSystemProvider::getFileSystem(llvm::NoneType) const' [-Woverloaded-virtual]
       getFileSystem(llvm::NoneType) const override;
       ^~~~~~~~~~~~~
uabelho added inline comments.Jun 23 2020, 2:06 AM
clang-tools-extra/clangd/Preamble.cpp
242

A warning here too

23:51:14 /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/sdk_1_20_ki_dev_test/clang-tools-extra/clangd/Preamble.cpp:234:5: warning:   by 'virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> clang::clangd::{anonymous}::scanPreamble(llvm::StringRef, const clang::tooling::CompileCommand&)::EmptyFS::view(llvm::NoneType) const' [-Woverloaded-virtual]
23:51:14      view(llvm::NoneType) const override {
23:51:14      ^~~~

thanks for the info @uabelho!

this looks like a dormant warning though, as StringRef is not implicitly convertible to NoneType (and vice-versa) hence anyone trying to make use of the hidden overload would get a hard compile error anyways.
Moreover this class is mostly accessed through a base pointer, hence name hiding in derived classes isn't really an issue (for most of the production code).

Also the warning itself seems to be noisy https://gcc.gnu.org/bugzilla/show_bug.cgi?id=20423. Interesting this seems to be only enabled for clang and nothing else, I wonder how it is decided.
Unfortunately history doesn't tell much https://github.com/llvm/llvm-project/blame/master/clang/CMakeLists.txt#L396.

There are 5 derived classes (3 of them are in tests), so just putting a using declaration to un-hide the overload seems too disruptive.
Again renaming the endpoints (and possibly changing the signature) just to suppress this warning also doesn't seem so nice.

I would rather like to turn-off this warning for at least gcc, assuming this is not specific to that version. Can you check if you see the warning with different versions of gcc?

Can you check if you see the warning with different versions of gcc?

Originally I got them with 7.4.0 and now I tried with 9.3.0 and got them there as well.

bjope added a subscriber: bjope.Jun 25 2020, 2:13 AM

I think the warnings can be hidden by something like this (I'm no expert in this area though, but it seems like this technique has been used a number of times before in llvm, so hopefully it applies here as well):

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index ca0a76db78f4..1970541bc56a 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -230,6 +230,7 @@ llvm::Expected<ScannedPreamble>
 scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
   class EmptyFS : public ThreadsafeFS {
   public:
+    using ThreadsafeFS::view;
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
     view(llvm::NoneType) const override {
       return new llvm::vfs::InMemoryFileSystem;
diff --git a/clang-tools-extra/clangd/support/ThreadsafeFS.h b/clang-tools-extra/clangd/support/ThreadsafeFS.h
index aa6825fb3999..eb9016bad201 100644
--- a/clang-tools-extra/clangd/support/ThreadsafeFS.h
+++ b/clang-tools-extra/clangd/support/ThreadsafeFS.h
@@ -42,6 +42,7 @@ public:
 
 class RealThreadsafeFS : public ThreadsafeFS {
 public:
+  using ThreadsafeFS::view;
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
       view(llvm::NoneType) const override;
 };

at least I get no warnings when building clangd (haven't checked the unittests, but I don't care about warnings in those right now).

Quuxplusone added inline comments.
clang-tools-extra/clangd/support/FSProvider.h
37

Re how to fix the GCC warning: I haven't fully understood this code yet (because it's hard to understand! :)) but I think what you're trying to do here is something like

class FileSystemProvider {
public:
    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
    getFileSystem(std::optional<PathRef> CWD) const {
        if (CWD.has_value()) {
            return this->doGetDefaultFileSystem();  // virtual call
        } else {
            return this->doGetFileSystem(CWD);  // virtual call
        }
    }
private:
    virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
    doGetDefaultFileSystem() const = 0;
    virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
    doGetFileSystem(PathRef) const;
};

And then in class RealFileSystemProvider, you want to override doGetDefaultFileSystem() while leaving doGetFileSystem untouched. Does that basically reflect your intent here?

And then in class RealFileSystemProvider, you want to override doGetDefaultFileSystem() while leaving doGetFileSystem untouched. Does that basically reflect your intent here?

Yes, except with fewer confusing names :-)

The name used now (there was a rename patch following this) is ThreadsafeFS::view - it took us a couple of years to work out what this abstraction really was.

The purpose of using the same name for the two public overloads is that they do the same thing. The purpose of using the same name for the public no-CWD version and the version to be overridden is that they *are* the same thing. These are more important reasons than the idea that non-existent non-polymorphic callers will miss an overload.

("More important" subjectively, but code always ends up stamped with the taste of the people working on it...)

Oops, I misread and forgot one point. The public interface can't (only) take optional<StringRef> because that's not implicitly convertible from a std::string which is what we usually want to pass. Thus the silly overloads, which we were going to fix with a more careful overload set approximating optional<StringRef> once we hit a case where we didn't know statically whether we had a working directory or not.

(Didn't push for it to be in this patch because getting overload sets right is hard)