[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham planned changes to this revision.
bnbarham added a comment.

Blocked on the dependent


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

In D121425#3384492 , @dexonsmith 
wrote:

> Can you be more detailed about the overall state at the time of `cat`, which 
> causes it to fail?

Sure. I think there's also some confusion with names here but I'm generally 
trying to use the names as they are used in the overlay YAML or the name of the 
members in RedirectingFS.

"external-contents" is just the path that we're mapping to, so in my example 
above (/a/foo -> bar), "bar" is what I was referring to by "external-contents". 
So let's use that again -

  OverlayFileSystem
RedirectingFileSystem
  /a/foo -> bar
  
RealFileSystem
  /a/bar
  /b/bar

After D121423 , `cd /a` will result in the 
following -

- OverlayFS: CWD set to `/a`
- RedirectingFS and RealFS: CWD unchanged, so let's assume they were `/` at the 
time of creation and that's what they are now

`cat foo` -

- OverlayFS canonicalises to `/a/foo` and passes that to `openFileForRead` in 
RedirectingFS
- RedirectingFS has a `/a/foo` -> `bar` mapping, so it now runs 
`openFileForRead` on its ExternalFS (which is the RealFS)
- RealFS is at `/` and there is no `/bar` and thus fails
- RedirectingFS also fails
- Back in OverlayFS now, and because we failed we now try RealFS with `/a/foo`
- RealFS has no `/a/foo`, it fails
- Back in OverlayFS, all FS have now failed, return a failure

How this used to work really depends on when we're talking about . Prior to 
D121423  the FS would have looked like:

  RedirectingFileSystem
/a/foo -> bar
  ExternalFS
RealFileSystem
  /a/bar
  /b/bar

Prior to Jonas' change, `setCWD` on RedirectingFS would run `setCWD` on 
`ExternalFS`. So `cd /a` would set CWD to `/a` for both RedirectingFS and 
RealFileSystem. Then `cat /a/foo` would give `bar` and `openFileForRead` on 
`ExternalFS` (RealFS) (which has `/a` CWD) would open `/a/bar`.

After Jonas' *and* Keith's change, `setCWD` no longer runs on `ExternalFS` but 
instead the path was canonicalised before sending it along. So `/a/foo` maps to 
`bar` as before, but then it would be canonicalised using `RedirectingFS` CWD 
and thus do an open for `/a/bar` on `ExternalFS`.

In D121425#3384499 , @dexonsmith 
wrote:

> (Would this mean resolving everything in the `.yaml` file eagerly at launch? 
> That sounds a bit scary...)

Yes, depending on what you mean by "resolving". All I mean is "prefix relative 
paths with CWD". The main issue with doing this is that it prohibits 
canonicalising paths to a virtual CWD, since you wouldn't be able to set CWD to 
a virtual path before making the overlay. Perhaps that's what you mean by "a 
bit scary" though.

We currently prefix relative paths with `ExternalContentsPrefixDir` if 
`overlay-relative: true` is set. That path is a combination of CWD + the 
directory to the overlay. This is to handle overlays that have absolute paths 
that we want to remap (specifically for the crash dump use case I believe). But 
if `overlay-relative: false` is set then we just canonicalise the path on 
open/etc.

I just ran a few tests with the frontend though:

- By default, paths come in relative and will become absolute from the 
process-wide CWD
- If `-working-directory` *is* set then paths come in absolute from 
`FileManager` - `setCWD` is never run

I'm not sure if there's a good reason it's done this way or it's just that CWD 
was only added to FileSystem relatively recently, but it does mean that that 
we're actually canonicalising the paths with the process-wide CWD regardless at 
the moment (at least from the frontend). This isn't the case if something was 
to use the API directly, but in that case I'm not sure that using the current 
CWD makes any more sense than using the CWD when the overlay is created. The 
semantics would be less confusing if that was the case (IMO), but again would 
prohibit canonicalising to a virtual CWD.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D121425#3384479 , @bnbarham wrote:

> In D121425#3384188 , @bnbarham 
> wrote:
>
>> There's two failing tests with this change:
>>
>> - VFSFromYAMLTest.ReturnsExternalPathVFSHit
>> - VFSFromYAMLTest.ReturnsInternalPathVFSHit
>>
>> Apparently we allow relative paths in external-contents *without* specifying 
>> `overlay-relative: true`. In this case the relative paths are resolved based 
>> on the CWD at the time of the operation. Do we really want that? I believe 
>> this wouldn't have worked prior to https://reviews.llvm.org/D109128 as there 
>> was no canonicalising before then as well. @keith was that change 
>> intentional? If we want it then it will *require* setting CWD on each FS in 
>> OverlayFS, which I was really hoping to avoid.
>
> I spoke to Keith offline. This has always worked - it previously worked by 
> `RedirectingFileSystem` setting CWD on `ExternalFS` when `setCWD` was called 
> on it. It's also important to keep supporting as it's used in Bazel 
> (https://github.com/bazelbuild/rules_swift/blob/c1d7d1df6969c2675c7826ecf1202d78016b1753/swift/internal/vfsoverlay.bzl#L41-L55).
>
> I'm hoping I can fix this by resolving the paths when the overlay is created. 
> I'll see if that works (it'll depend on when -working-directory is actually 
> used by the frontend).

(Would this mean resolving everything in the `.yaml` file eagerly at launch? 
That sounds a bit scary...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D121425#3384188 , @bnbarham wrote:

> There's two failing tests with this change:
>
> - VFSFromYAMLTest.ReturnsExternalPathVFSHit
> - VFSFromYAMLTest.ReturnsInternalPathVFSHit
>
> Apparently we allow relative paths in external-contents *without* specifying 
> `overlay-relative: true`. In this case the relative paths are resolved based 
> on the CWD at the time of the operation. Do we really want that? I believe 
> this wouldn't have worked prior to https://reviews.llvm.org/D109128 as there 
> was no canonicalising before then as well. @keith was that change 
> intentional? If we want it then it will *require* setting CWD on each FS in 
> OverlayFS, which I was really hoping to avoid.

I'm not sure how external-contents factors into this (shouldn't that just 
affect the observed file path, not whether the lookup succeeds?).

Regardless, here's the testcase again:

  OverlayFileSystem
RedirectingFileSystem
  /a/foo -> bar
  
RealFileSystem
  /a/bar
  /b/bar

I think that should behave like:

  InMemoryFileSystem
/a/bar
/a/foo -> bar (link, with "magic" to sometimes leak path)
/b/bar
  
  cd /a # succeeds
  cat foo # succeeds (get content of /a/bar; report path of /a/bar or /a/foo)
  
  cd /b # succeeds
  cat foo # fails

Is the model correct?

If so, I think these are the things we need to get right (not just for that 
test case):

1. Return error on operations that should fail.
2. Get the right content what `cat` succeeds.
3. Reporting the "right" paths for files that are opened (depends on external 
contents, etc.).
4. Keeping the "view" / state consistent when the underlying filesystems 
changes state.

Does that breakdown make sense?

If so, can you clarify which part is failing? (Not sure if it's (3) or (4)... 
or maybe some (5).)

Or another way in: I don't fully understand why this fails:

>   cd /a
>   cat foo # fails since OverlayFS::setCWD doesn't set CWD on the underlying FS

Can you be more detailed about the overall state at the time of `cat`, which 
causes it to fail?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

In D121425#3384188 , @bnbarham wrote:

> There's two failing tests with this change:
>
> - VFSFromYAMLTest.ReturnsExternalPathVFSHit
> - VFSFromYAMLTest.ReturnsInternalPathVFSHit
>
> Apparently we allow relative paths in external-contents *without* specifying 
> `overlay-relative: true`. In this case the relative paths are resolved based 
> on the CWD at the time of the operation. Do we really want that? I believe 
> this wouldn't have worked prior to https://reviews.llvm.org/D109128 as there 
> was no canonicalising before then as well. @keith was that change 
> intentional? If we want it then it will *require* setting CWD on each FS in 
> OverlayFS, which I was really hoping to avoid.

I spoke to Keith offline. This has always worked - it previously worked by 
`RedirectingFileSystem` setting CWD on `ExternalFS` when `setCWD` was called on 
it. It's also important to keep supporting as it's used in Bazel 
(https://github.com/bazelbuild/rules_swift/blob/c1d7d1df6969c2675c7826ecf1202d78016b1753/swift/internal/vfsoverlay.bzl#L41-L55).

I'm hoping I can fix this by resolving the paths when the overlay is created. 
I'll see if that works (it'll depend on when -working-directory is actually 
used by the frontend).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

There's two failing tests with this change:

- VFSFromYAMLTest.ReturnsExternalPathVFSHit
- VFSFromYAMLTest.ReturnsInternalPathVFSHit

Apparently we allow relative paths in external-contents *without* specifying 
`overlay-relative: true`. In this case the relative paths are resolved based on 
the CWD at the time of the operation. Do we really want that? I believe this 
wouldn't have worked prior to https://reviews.llvm.org/D109128 as there was no 
canonicalising before then as well. @keith was that change intentional? If we 
want it then it will *require* setting CWD on each FS in OverlayFS, which I was 
really hoping to avoid.

To give the concrete case:

  OverlayFileSystem
RedirectingFileSystem
  /a/foo -> bar
RealFileSystem
  /a/bar
  /b/bar
  
  cd /a
  cat foo # /a/bar
  
  cd /b # would fail previously since it doesn't exist in RedirectingFS but 
would still setCWD for RealFS
  cat foo # and thus return /a/bar since RedirectingFS is still in /a

This now fails completely because OverlayFS doesn't setCWD on the underlying 
filesystems. So RedirectingFS has the CWD of whatever ExternalFS was at 
creation. In D121424  it will fail because 
there's no longer any canonicalise after mapping the path in RedirectingFS (I 
assumed this was unintentional and missed the test case).

If we want to allow this then I think we'll need to go with the previous plan 
for OverlayFS, ie. keep track of filesystems that setCWD failed on and don't 
call them at all until they succeed again. That then doesn't allow certain 
cases that the current method allows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

[Ben Barham via Phabricator via cfe-commits]

bnbarham updated this revision to Diff 415615.
bnbarham edited the summary of this revision.
bnbarham added a comment.
Herald added subscribers: cfe-commits, lldb-commits, carlosgalvezp.
Herald added projects: clang, LLDB, clang-tools-extra.

Re-order to be before D121424  and merge with 
D121426 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121425/new/

https://reviews.llvm.org/D121425

Files:
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/directory.c
  clang/test/VFS/multiple-overlays.c
  clang/test/VFS/vfsroot-with-overlay.c
  lldb/source/Commands/CommandObjectReproducer.cpp
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/tools/dsymutil/Reproducer.cpp
  llvm/tools/dsymutil/Reproducer.h
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -14,6 +14,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
+#include "llvm/Testing/Support/Error.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -2571,7 +2572,7 @@
 }
 
 TEST_F(VFSFromYAMLTest, WorkingDirectory) {
-  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem());
   Lower->addDirectory("//root/");
   Lower->addDirectory("//root/foo");
   Lower->addRegularFile("//root/foo/a");
@@ -2593,6 +2594,7 @@
   "}",
   Lower);
   ASSERT_NE(FS.get(), nullptr);
+
   std::error_code EC = FS->setCurrentWorkingDirectory("//root/bar");
   ASSERT_FALSE(EC);
 
@@ -2621,6 +2623,14 @@
   ASSERT_TRUE(WorkingDir);
   EXPECT_EQ(*WorkingDir, "//root/");
 
+  Status = FS->status("bar/a");
+  ASSERT_FALSE(Status.getError());
+  EXPECT_TRUE(Status->exists());
+
+  Status = FS->status("foo/a");
+  ASSERT_FALSE(Status.getError());
+  EXPECT_TRUE(Status->exists());
+
   EC = FS->setCurrentWorkingDirectory("bar");
   ASSERT_FALSE(EC);
   WorkingDir = FS->getCurrentWorkingDirectory();
@@ -2710,43 +2720,6 @@
   EXPECT_TRUE(Status->exists());
 }
 
-TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) {
-  IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem());
-  Lower->addDirectory("//root/");
-  Lower->addDirectory("//root/foo");
-  Lower->addRegularFile("//root/foo/a");
-  Lower->addRegularFile("//root/foo/b");
-  Lower->addRegularFile("//root/c");
-  IntrusiveRefCntPtr FS = getFromYAMLString(
-  "{ 'use-external-names': false,\n"
-  "  'roots': [\n"
-  "{\n"
-  "  'type': 'directory',\n"
-  "  'name': '//root/bar',\n"
-  "  'contents': [ {\n"
-  "  'type': 'file',\n"
-  "  'name': 'a',\n"
-  "  'external-contents': '//root/foo/a'\n"
-  "}\n"
-  "  ]\n"
-  "}\n"
-  "]\n"
-  "}",
-  Lower);
-  ASSERT_NE(FS.get(), nullptr);
-  std::error_code EC = FS->setCurrentWorkingDirectory("//root/");
-  ASSERT_FALSE(EC);
-  ASSERT_NE(FS.get(), nullptr);
-
-  llvm::ErrorOr Status = FS->status("bar/a");
-  ASSERT_FALSE(Status.getError());
-  EXPECT_TRUE(Status->exists());
-
-  Status = FS->status("foo/a");
-  ASSERT_FALSE(Status.getError());
-  EXPECT_TRUE(Status->exists());
-}
-
 TEST_F(VFSFromYAMLTest, VirtualWorkingDirectory) {
   IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem());
   Lower->addDirectory("//root/");
@@ -3207,3 +3180,168 @@
 "  DummyFileSystem (RecursiveContents)\n",
 Output);
 }
+
+static std::unique_ptr
+getVFSOrNull(ArrayRef YAMLOverlays,
+ IntrusiveRefCntPtr ExternalFS) {
+  SmallVector OverlayRefs;
+  for (const auto  : YAMLOverlays) {
+OverlayRefs.emplace_back(Overlay, "");
+  }
+
+  auto ExpectedFS = vfs::getVFSFromYAMLs(OverlayRefs, ExternalFS);
+  if (auto Err = ExpectedFS.takeError()) {
+consumeError(std::move(Err));
+return nullptr;
+  }
+  return std::move(*ExpectedFS);
+}
+
+static std::string createSimpleOverlay(StringRef RedirectKind, StringRef From,
+   StringRef To) {
+  return ("{\n"
+  "  'version': 0,\n"
+  "  'redirecting-with': '" +
+  RedirectKind +
+  "'\n"
+  "  'roots': [\n"
+  " {\n"
+  "   'type': 'directory-remap',\n"
+  "   'name': '" +
+  From +
+  "',\n"
+  "   'external-contents': '" +
+  To +
+  "',\n"
+  "   }]\n"
+  " }"
+  "  ]")
+  .str();
+}
+
+// Make sure that overlays are not transitive. Given A ->