[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei added a comment.

In D137473#3993584 , @bnbarham wrote:

> LGTM. If you could put up a PR after to fix the use of 
> `sys::fs::make_absolute` that would be appreciated 🙇.

Thanks, I will start working on the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa903ecb4a26d: [vfs] Allow root paths relative to the 
vfsoverlay YAML file (authored by haowei).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1456,18 +1456,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1853,6 +1855,69 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  // On Windows, with overlay-relative set to true, the relative
+  // path in external-contents field will be prepend by OverlayDir
+  // with native path separator, regardless of the actual path separator
+  // used in YAMLFilePath field.
+#ifndef _WIN32
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+#else
+  IntrusiveRefCntPtr LowerWindows(new DummyFileSystem());
+  LowerWindows->addDirectory("root\\foo\\bar");
+  LowerWindows->addRegularFile("root\\foo\\bar\\a");
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ LowerWindows, "root\\foo\\bar\\overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("root\\foo\\bar\\b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("root\\foo\\bar\\a", S->getName());
+#endif
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
==

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via cfe-commits]

bnbarham accepted this revision.
bnbarham added a comment.
This revision is now accepted and ready to land.

LGTM. If you could put up a PR after to fix the use of `sys::fs::make_absolute` 
that would be appreciated 🙇.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 482660.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1456,18 +1456,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1853,6 +1855,69 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  // On Windows, with overlay-relative set to true, the relative
+  // path in external-contents field will be prepend by OverlayDir
+  // with native path separator, regardless of the actual path separator
+  // used in YAMLFilePath field.
+#ifndef _WIN32
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+#else
+  IntrusiveRefCntPtr LowerWindows(new DummyFileSystem());
+  LowerWindows->addDirectory("root\\foo\\bar");
+  LowerWindows->addRegularFile("root\\foo\\bar\\a");
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ LowerWindows, "root\\foo\\bar\\overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("root\\foo\\bar\\b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("root\\foo\\bar\\a", S->getName());
+#endif
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1350,32 +1350,51 @@
   if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
 

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 482658.
haowei added a comment.

Rebased the change to solve the merge conflicts on the bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1456,18 +1456,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1853,6 +1855,69 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  // On Windows, with overlay-relative set to true, the relative
+  // path in external-contents field will be prepend by OverlayDir
+  // with native path separator, regardless of the actual path separator
+  // used in YAMLFilePath field.
+#ifndef _WIN32
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+#else
+  IntrusiveRefCntPtr LowerWindows(new DummyFileSystem());
+  LowerWindows->addDirectory("root\\foo\\bar");
+  LowerWindows->addRegularFile("root\\foo\\bar\\a");
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ LowerWindows, "root\\foo\\bar\\overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("root\\foo\\bar\\b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("root\\foo\\bar\\a", S->getName());
+#endif
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1350,32 +1

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei added a comment.

I changed some comments and add more details.
If there is no objection, can I get an approval on this change so I can land 
it? It will unblock our development on Windows cross compilation support.




Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1933-1935
 path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
  ? sys::path::Style::posix
  : sys::path::Style::windows_backslash;

phosek wrote:
> haowei wrote:
> > bnbarham wrote:
> > > `windows_slash` is going to be `windows_backslash` here. I assume this 
> > > was fine since `sys::fs::make_absolute(Name)` would always return the 
> > > native style, but that isn't the case now. It's also unfortunate we 
> > > calculate this again when `makeAbsolute` only just did it.
> > Thanks for pointing that out. I need to consider the windows forward slash 
> > case.
> > 
> > There is a bit in consistency in the VFS implementation. for Windows. we 
> > can use forward slashes('/') on Windows. Most Windows API/Syscalls accept 
> > it. But some APIs may not work if you mix '/' with '\' in the path on 
> > Windows. What RedirectFileSystem::makeAbsolute is trying to do is, it first 
> > determine the slashes used in the WorkDir(either from process working 
> > directory or from command line flags, or OverlayDir), in which it can be a 
> > forward slash on Windows. Then it uses the same separator when appending 
> > the relative directory.  Using sys::fs::make_absolute will break that. 
> > 
> > The reason that I use makeAbsolute here is that the OverlayDir will likely 
> > use forward slash on Windows if people uses CMake options 
> > LINK_FLAG="/vfsoverlay:C:/path/to/overlay.xml". In that case, 
> > sys::fs::make_absolute will generate some paths like C:/path/to/foo\\bar, 
> > which may cause issues.
> > 
> > I will rewrite this to consider the forward slashes.
> This was also discussed in D87732 which may provide some additional context.
I looked into the implementation of llvm path library and how 
`windows_backslash` and `windows_slash` are implemented and used. 
TL;DR,
The difference between `windows_backslash` and `windows_slash` is that when 
llvm::get_separator is called, it returns a different slash. The rest of the 
API behaviors are the same. So in this `VirtualFileSystem.cpp`, it doesn't 
matter if the path_style is assigned to 
sys::path::Style::windows_backslash or sys::path::Style::windows_slash here as 
the output of this function `parseEntry` will be the same.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32

haowei wrote:
> rnk wrote:
> > dexonsmith wrote:
> > > bnbarham wrote:
> > > > `separator` -> `separator`
> > > > 
> > > > As per my comment above, this is only true because we're either using 
> > > > posix or windows_backslash. I'm not sure what we really want here, 
> > > > should we always convert to native? Any thoughts @dexonsmith?
> > > > 
> > > > FWIW the non-windows case is identical to the above block. It'd be 
> > > > interesting to test `/` and `\` on both filesystems.
> > > I think we might need a Windows person to help answer; @rnk, can you 
> > > suggest someone to help answer @bnbarham's question?
> > I can try suggesting @hans, @compnerd, or @Bigcheese 
> The non-windows case enable the overlay-relative option. The above block 
> didn't. And that is where I discover the issue with overlay-relative always 
> uses native separator instead of trying to match OverlayDir.
> 
> The best approach for root-relative and overlay-relative option would 
> probably be matching the style of OverlayDir/WorkDir and unify the path 
> separator if they are mixed in a path.
> But this change will likely affects a few tests.
I corrected the typo.

I did some Windows API tests in the past week and I can confirm on Windows 10 
(but should be apply to all NT6 family Windows), the forward slashes "/" and 
backward slashes "\" are both legit path separators and they be mixed in a path 
in any combinations. They will behave properly and no errors will be reported. 
However, the cmd.exe and some other Windows program will have some issue if 
forward slashes is used or mixed with backward slashes because it looks like 
flag switches, probably because these programs parse the user Input and doing 
their own path processing before invoking Windows APIs.

In LLVM's case, since File IO will rely on Windows APIs, I think how my current 
code handling the path separators under relative path scenario is sufficient. 
It shouldn't bring new issues.

On Linux and other POSIX OS, the backward slash is a legit path character. 
e.g.
/foo/bar/\\a/b means 
```
foo
├- bar
  ├ `\a`
├ b
```
Therefore, if backward slashes are used in the overlay file under Linux

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 482644.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1452,18 +1452,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1849,6 +1851,69 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  // On Windows, with overlay-relative set to true, the relative
+  // path in external-contents field will be prepend by OverlayDir
+  // with native path separator, regardless of the actual path separator
+  // used in YAMLFilePath field.
+#ifndef _WIN32
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+#else
+  IntrusiveRefCntPtr LowerWindows(new DummyFileSystem());
+  LowerWindows->addDirectory("root\\foo\\bar");
+  LowerWindows->addRegularFile("root\\foo\\bar\\a");
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ LowerWindows, "root\\foo\\bar\\overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("root\\foo\\bar\\b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("root\\foo\\bar\\a", S->getName());
+#endif
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1350,32 +1350,51 @@
   if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
 

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1933-1935
 path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
  ? sys::path::Style::posix
  : sys::path::Style::windows_backslash;

haowei wrote:
> bnbarham wrote:
> > `windows_slash` is going to be `windows_backslash` here. I assume this was 
> > fine since `sys::fs::make_absolute(Name)` would always return the native 
> > style, but that isn't the case now. It's also unfortunate we calculate this 
> > again when `makeAbsolute` only just did it.
> Thanks for pointing that out. I need to consider the windows forward slash 
> case.
> 
> There is a bit in consistency in the VFS implementation. for Windows. we can 
> use forward slashes('/') on Windows. Most Windows API/Syscalls accept it. But 
> some APIs may not work if you mix '/' with '\' in the path on Windows. What 
> RedirectFileSystem::makeAbsolute is trying to do is, it first determine the 
> slashes used in the WorkDir(either from process working directory or from 
> command line flags, or OverlayDir), in which it can be a forward slash on 
> Windows. Then it uses the same separator when appending the relative 
> directory.  Using sys::fs::make_absolute will break that. 
> 
> The reason that I use makeAbsolute here is that the OverlayDir will likely 
> use forward slash on Windows if people uses CMake options 
> LINK_FLAG="/vfsoverlay:C:/path/to/overlay.xml". In that case, 
> sys::fs::make_absolute will generate some paths like C:/path/to/foo\\bar, 
> which may cause issues.
> 
> I will rewrite this to consider the forward slashes.
This was also discussed in D87732 which may provide some additional context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1922
+  EC = FS->makeAbsolute(FullPath, Name);
+  Name = canonicalize(Name);
+} else {

bnbarham wrote:
> Why the canonicalization?
If the Name here somehow has something like //foo/./../bar, it will cause match 
failures. Canonicalize function removes these dots.
The setOverlayFileDir does not canonicalize the dir so this is quite common to 
see.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1933-1935
 path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
  ? sys::path::Style::posix
  : sys::path::Style::windows_backslash;

bnbarham wrote:
> `windows_slash` is going to be `windows_backslash` here. I assume this was 
> fine since `sys::fs::make_absolute(Name)` would always return the native 
> style, but that isn't the case now. It's also unfortunate we calculate this 
> again when `makeAbsolute` only just did it.
Thanks for pointing that out. I need to consider the windows forward slash case.

There is a bit in consistency in the VFS implementation. for Windows. we can 
use forward slashes('/') on Windows. Most Windows API/Syscalls accept it. But 
some APIs may not work if you mix '/' with '\' in the path on Windows. What 
RedirectFileSystem::makeAbsolute is trying to do is, it first determine the 
slashes used in the WorkDir(either from process working directory or from 
command line flags, or OverlayDir), in which it can be a forward slash on 
Windows. Then it uses the same separator when appending the relative directory. 
 Using sys::fs::make_absolute will break that. 

The reason that I use makeAbsolute here is that the OverlayDir will likely use 
forward slash on Windows if people uses CMake options 
LINK_FLAG="/vfsoverlay:C:/path/to/overlay.xml". In that case, 
sys::fs::make_absolute will generate some paths like C:/path/to/foo\\bar, which 
may cause issues.

I will rewrite this to consider the forward slashes.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32

rnk wrote:
> dexonsmith wrote:
> > bnbarham wrote:
> > > `separator` -> `separator`
> > > 
> > > As per my comment above, this is only true because we're either using 
> > > posix or windows_backslash. I'm not sure what we really want here, should 
> > > we always convert to native? Any thoughts @dexonsmith?
> > > 
> > > FWIW the non-windows case is identical to the above block. It'd be 
> > > interesting to test `/` and `\` on both filesystems.
> > I think we might need a Windows person to help answer; @rnk, can you 
> > suggest someone to help answer @bnbarham's question?
> I can try suggesting @hans, @compnerd, or @Bigcheese 
The non-windows case enable the overlay-relative option. The above block 
didn't. And that is where I discover the issue with overlay-relative always 
uses native separator instead of trying to match OverlayDir.

The best approach for root-relative and overlay-relative option would probably 
be matching the style of OverlayDir/WorkDir and unify the path separator if 
they are mixed in a path.
But this change will likely affects a few tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Reid Kleckner via Phabricator via cfe-commits]

rnk added subscribers: hans, compnerd, Bigcheese.
rnk added inline comments.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32

dexonsmith wrote:
> bnbarham wrote:
> > `separator` -> `separator`
> > 
> > As per my comment above, this is only true because we're either using posix 
> > or windows_backslash. I'm not sure what we really want here, should we 
> > always convert to native? Any thoughts @dexonsmith?
> > 
> > FWIW the non-windows case is identical to the above block. It'd be 
> > interesting to test `/` and `\` on both filesystems.
> I think we might need a Windows person to help answer; @rnk, can you suggest 
> someone to help answer @bnbarham's question?
I can try suggesting @hans, @compnerd, or @Bigcheese 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

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

dexonsmith added a subscriber: rnk.
dexonsmith added inline comments.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32

bnbarham wrote:
> `separator` -> `separator`
> 
> As per my comment above, this is only true because we're either using posix 
> or windows_backslash. I'm not sure what we really want here, should we always 
> convert to native? Any thoughts @dexonsmith?
> 
> FWIW the non-windows case is identical to the above block. It'd be 
> interesting to test `/` and `\` on both filesystems.
I think we might need a Windows person to help answer; @rnk, can you suggest 
someone to help answer @bnbarham's question?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1922
+  EC = FS->makeAbsolute(FullPath, Name);
+  Name = canonicalize(Name);
+} else {

Why the canonicalization?



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1933-1935
 path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
  ? sys::path::Style::posix
  : sys::path::Style::windows_backslash;

`windows_slash` is going to be `windows_backslash` here. I assume this was fine 
since `sys::fs::make_absolute(Name)` would always return the native style, but 
that isn't the case now. It's also unfortunate we calculate this again when 
`makeAbsolute` only just did it.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32

`separator` -> `separator`

As per my comment above, this is only true because we're either using posix or 
windows_backslash. I'm not sure what we really want here, should we always 
convert to native? Any thoughts @dexonsmith?

FWIW the non-windows case is identical to the above block. It'd be interesting 
to test `/` and `\` on both filesystems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 475884.
haowei marked 2 inline comments as done.
haowei added a comment.

Fixing Windows test failures.
overlay-relative flag will always using native path separator, therefore, it 
needs a separate base FS and OverlayYAML file setup on Windows. This diff adds 
this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1452,18 +1452,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1849,6 +1851,69 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  // On Windows, with overlay-relative set to true, the relative
+  // path in external-contents field will be prepend by OverlayDir
+  // with native path seperator, regardless of the actual path seperator
+  // used in YAMLFilePath field.
+#ifndef _WIN32
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+#else
+  IntrusiveRefCntPtr LowerWindows(new DummyFileSystem());
+  LowerWindows->addDirectory("root\\foo\\bar");
+  LowerWindows->addRegularFile("root\\foo\\bar\\a");
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ LowerWindows, "root\\foo\\bar\\overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("root\\foo\\bar\\b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("root\\foo\\bar\\a", S->getName());
+#endif
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSyste

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei marked 2 inline comments as done.
haowei added inline comments.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:1010
 
+  std::error_code makeAbsolute(StringRef WorkingDir,
+   SmallVectorImpl &Path) const;

bnbarham wrote:
> Should be private IMO
Moved it to private section and added comments



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1369
   // append Path ourselves.
+  if (!sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+  !sys::path::is_absolute(WorkingDir,

bnbarham wrote:
> Did you find this was needed? It's already checked by the normal 
> `makeAbsolute` (which checks this already) and the only other caller is when 
> we're using the overlay directory path, which should always be absolute.
No, it is not needed for now. As the OverlayDir is always abs when it was set. 
The normal makeAbsolute function checks "Path" param but not checking the 
returned WorkDir, which is OK because the return value from 
getCurrentWorkingDirectory will be abs or empty.

It is more to be future proof if someone decide to use this function for other 
purpose and putting a non-abs path in WorkingDir field will causes unexpected 
behaviors.

I also need to exclude empty WorkingDir though because in unit tests, the 
WorkingDir is always empty.




Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1913
+ RedirectingFileSystem::RootRelativeKind::OverlayDir) {
+SmallString<256> FullPath = FS->getOverlayFileDir();
+assert(!FullPath.empty() && "Overlay file directory must exist");

bnbarham wrote:
> This is unused now. Maybe we should just merge the `else if` and `else` 
> branches and just grab either `getOverlayFileDir` or 
> `getCurrentWorkingDirectory` depending on `RootRelative`. They should 
> otherwise be identical.
I merged these 2 else block.
I have to use sys::fs::make_absolute function for now as using FS's CWD will 
break a few tests. It is better to address these test in a separate patch.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1875
+
+  IntrusiveRefCntPtr FSOverlayRelative =
+  getFromYAMLString("{\n"

bnbarham wrote:
> Just override the previous `FS`/`S` IMO - that way we avoid the accidental 
> `FS->status` a few lines down 😅
Thanks for pointing that out 😅


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 475591.
haowei added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1452,18 +1452,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1849,6 +1851,44 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  FS = getFromYAMLString("{\n"
+ "  'case-sensitive': false,\n"
+ "  'overlay-relative': true,\n"
+ "  'root-relative': 'overlay-dir',\n"
+ "  'roots': [\n"
+ "{ 'name': 'b', 'type': 'file',\n"
+ "  'external-contents': 'a'\n"
+ "}\n"
+ "  ]\n"
+ "}",
+ Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FS.get(), nullptr);
+  S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1356,22 +1356,33 @@
   if (!WorkingDir)
 return WorkingDir.getError();
 
+  return makeAbsolute(WorkingDir.get(), Path);
+}
+
+std::error_code
+RedirectingFileSystem::makeAbsolute(StringRef WorkingDir,
+SmallVectorImpl &Path) const {
   // We can't use sys::fs::make_absolute because that assumes the path style
   // is native and there is no way to override that.  Since we know WorkingDir
   // is absolute, we can use it to determine which style we actually have and
   // append Path ourselves.
+  if (!WorkingDir.empty() &&
+  !sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+  !sys::path::is_absolute(WorkingDir,
+  sys::path::Style::windows_backslash)) {
+return std::error_code();
+  }
   sys::path::Style style = sys::path::Style::windows_backslash;
-  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::posix)) {
+  if (sys::path::is_absolute(WorkingDir, sys::path::Style::posix)) {
 style = sys::path::Style::posix;
   } else {
 // Distinguish between windows_backslash and windows_slash; getExistingStyle
 // returns posix for a path with windows_slash.
-if (getExist

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 475574.
haowei added a comment.

I have to revert back to use process cwd instead of base fs cwd. There are a 
few tests relied the behavior of using process cwd that I need to take a closer 
look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1452,18 +1452,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1849,6 +1851,45 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  IntrusiveRefCntPtr FSOverlayRelative =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'overlay-relative': true,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': 'a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FSOverlayRelative.get(), nullptr);
+  ErrorOr SOverlayRelative = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(SOverlayRelative.getError());
+  EXPECT_EQ("//root/foo/bar/a", SOverlayRelative->getName());
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1356,22 +1356,33 @@
   if (!WorkingDir)
 return WorkingDir.getError();
 
+  return makeAbsolute(WorkingDir.get(), Path);
+}
+
+std::error_code
+RedirectingFileSystem::makeAbsolute(StringRef WorkingDir,
+SmallVectorImpl &Path) const {
   // We can't use sys::fs::make_absolute because that assumes the path style
   // is native and there is no way to override that.  Since we know WorkingDir
   // is absolute, we can use it to determine which style we actually have and
   // append Path ourselves.
+  if (!WorkingDir.empty() &&
+  !sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+  !sys::path::is_absolute(WorkingDir,
+  sys::path::Style::windows_backslash)) {
+return std::error_code();
+  }
   sys::path::Style style = sys::path::Style::windows_backslash;
-  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::posix)) {
+  if (sys::path::is_absolute(Wo

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:1010
 
+  std::error_code makeAbsolute(StringRef WorkingDir,
+   SmallVectorImpl &Path) const;

Should be private IMO



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1369
   // append Path ourselves.
+  if (!sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+  !sys::path::is_absolute(WorkingDir,

Did you find this was needed? It's already checked by the normal `makeAbsolute` 
(which checks this already) and the only other caller is when we're using the 
overlay directory path, which should always be absolute.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1913
+ RedirectingFileSystem::RootRelativeKind::OverlayDir) {
+SmallString<256> FullPath = FS->getOverlayFileDir();
+assert(!FullPath.empty() && "Overlay file directory must exist");

This is unused now. Maybe we should just merge the `else if` and `else` 
branches and just grab either `getOverlayFileDir` or 
`getCurrentWorkingDirectory` depending on `RootRelative`. They should otherwise 
be identical.



Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1875
+
+  IntrusiveRefCntPtr FSOverlayRelative =
+  getFromYAMLString("{\n"

Just override the previous `FS`/`S` IMO - that way we avoid the accidental 
`FS->status` a few lines down 😅


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 475347.
haowei added a comment.

Unit test failures under Windows should be fixed now.

Root->name will also use base FS's current directory instead of process current 
directory now. Please take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1452,18 +1452,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1849,6 +1851,45 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  IntrusiveRefCntPtr FSOverlayRelative =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'overlay-relative': true,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': 'a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FSOverlayRelative.get(), nullptr);
+  ErrorOr SOverlayRelative = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(SOverlayRelative.getError());
+  EXPECT_EQ("//root/foo/bar/a", SOverlayRelative->getName());
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1356,22 +1356,32 @@
   if (!WorkingDir)
 return WorkingDir.getError();
 
+  return makeAbsolute(WorkingDir.get(), Path);
+}
+
+std::error_code
+RedirectingFileSystem::makeAbsolute(StringRef WorkingDir,
+SmallVectorImpl &Path) const {
   // We can't use sys::fs::make_absolute because that assumes the path style
   // is native and there is no way to override that.  Since we know WorkingDir
   // is absolute, we can use it to determine which style we actually have and
   // append Path ourselves.
+  if (!sys::path::is_absolute(WorkingDir, sys::path::Style::posix) &&
+  !sys::path::is_absolute(WorkingDir,
+  sys::path::Style::windows_backslash)) {
+return std::error_code();
+  }
   sys::path::Style style = sys::path::Style::windows_backslash;
-  if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::posix)) {
+  if (sys::path::is_absolute(WorkingDir, sys::path::Sty

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei added inline comments.



Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',

haowei wrote:
> haowei wrote:
> > bnbarham wrote:
> > > bnbarham wrote:
> > > > I'd prefer a test without `overlay-relative` set to make it clear they 
> > > > don't depend on each other.
> > > There's also unit tests in 
> > > `llvm/unittests/Support/VirtualFileSystemTest.cpp` that you could add to.
> > I need some time to patch this unit test for testing the new option.
> > Existing functions in this unit test file does not support defining the 
> > YAMLFilePath.
> > 
> > I will update this patch when I finish.
> I updated the yaml file and add unit tests with overlay-relative set to true 
> and false.
I updated `llvm/unittests/Support/VirtualFileSystemTest.cpp` to include tests 
on `root-relative` option.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1903
+assert(!FullPath.empty() && "YAML file directory must exist");
+sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+Name = canonicalize(Name);

bnbarham wrote:
> haowei wrote:
> > bnbarham wrote:
> > > IMO both this and CWD should be using the base FS instead. VFS didn't 
> > > have a CWD previously, but now that it does it doesn't really make sense 
> > > to use the process wide CWD. Especially since `-working-directory` 
> > > doesn't change it.
> > Could you clarify a bit more about the "base FS" please? I am still quite 
> > new to the LLVM VFS system.  Which API should I use to get the appropriate 
> > working directory instead of the the process wide CWD?
> > 
> > I avoided changing the default behavior (relative to the process's current 
> > working directory) as I am a bit concerned breaking other people's use 
> > cases. 
> > 
> > I looked up a bit, the value "--working-directory" will be writen into 
> > "VFS->setCurrentWorkingDirectory()" Do you think it is a better idea to use 
> > this value instead of the process current working directory? Though it 
> > would still be a behavior change for users rely on process current working 
> > directories. 
> > Could you clarify a bit more about the "base FS" please?
> 
> By "base FS" I just meant the filesystem that's passed down when creating the 
> RedirectingFileSystem, ie. `ExternalFS` in `getVFSFromYAML`.
> 
> > Which API should I use to get the appropriate working directory instead of 
> > the the process wide CWD?
> 
> `ExternalFS->getCurrentWorkingDirectory()` and `ExternalFS->makeAbsolute` is 
> what I'd expect to be used here.
> 
> > Do you think it is a better idea to use this value instead of the process 
> > current working directory? Though it would still be a behavior change for 
> > users rely on process current working directories.
> 
> Yes, I think it's surprising that it currently *isn't* using it. It's just a 
> hold over from when the VFS didn't have the concept of CWD. I'd be happy with 
> accepting this patch as is, with a separate patch after that changes to using 
> the VFS CWD.
> Yes, I think it's surprising that it currently *isn't* using it. It's just a 
> hold over from when the VFS didn't have the concept of CWD. I'd be happy with 
> accepting this patch as is, with a separate patch after that changes to using 
> the VFS CWD.

I see, I can upload a follow up change after this one is landed. to use VFS CWD 
instead of process CWD here. It is also easier to revert if anything goes wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 474391.
haowei added a comment.

Add additional unit test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/unittests/Support/VirtualFileSystemTest.cpp

Index: llvm/unittests/Support/VirtualFileSystemTest.cpp
===
--- llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -1452,18 +1452,20 @@
 
   std::unique_ptr
   getFromYAMLRawString(StringRef Content,
-   IntrusiveRefCntPtr ExternalFS) {
+   IntrusiveRefCntPtr ExternalFS,
+   StringRef YAMLFilePath = "") {
 std::unique_ptr Buffer = MemoryBuffer::getMemBuffer(Content);
-return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
-  ExternalFS);
+return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, YAMLFilePath,
+  this, ExternalFS);
   }
 
   std::unique_ptr getFromYAMLString(
   StringRef Content,
-  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem()) {
+  IntrusiveRefCntPtr ExternalFS = new DummyFileSystem(),
+  StringRef YAMLFilePath = "") {
 std::string VersionPlusContent("{\n  'version':0,\n");
 VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
-return getFromYAMLRawString(VersionPlusContent, ExternalFS);
+return getFromYAMLRawString(VersionPlusContent, ExternalFS, YAMLFilePath);
   }
 
   // This is intended as a "XFAIL" for windows hosts.
@@ -1849,6 +1851,45 @@
   EXPECT_EQ(0, NumDiagnostics);
 }
 
+TEST_F(VFSFromYAMLTest, RootRelativeTest) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/foo/bar");
+  Lower->addRegularFile("//root/foo/bar/a");
+  IntrusiveRefCntPtr FS =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': '//root/foo/bar/a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+
+  ASSERT_NE(FS.get(), nullptr);
+  ErrorOr S = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(S.getError());
+  EXPECT_EQ("//root/foo/bar/a", S->getName());
+
+  IntrusiveRefCntPtr FSOverlayRelative =
+  getFromYAMLString("{\n"
+"  'case-sensitive': false,\n"
+"  'overlay-relative': true,\n"
+"  'root-relative': 'overlay-dir',\n"
+"  'roots': [\n"
+"{ 'name': 'b', 'type': 'file',\n"
+"  'external-contents': 'a'\n"
+"}\n"
+"  ]\n"
+"}",
+Lower, "//root/foo/bar/overlay");
+  ASSERT_NE(FSOverlayRelative.get(), nullptr);
+  ErrorOr SOverlayRelative = FS->status("//root/foo/bar/b");
+  ASSERT_FALSE(SOverlayRelative.getError());
+  EXPECT_EQ("//root/foo/bar/a", SOverlayRelative->getName());
+}
+
 TEST_F(VFSFromYAMLTest, ReturnsInternalPathVFSHit) {
   IntrusiveRefCntPtr BaseFS(
   new vfs::InMemoryFileSystem);
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1482,12 +1482,12 @@
   return Combined;
 }
 
-void RedirectingFileSystem::setExternalContentsPrefixDir(StringRef PrefixDir) {
-  ExternalContentsPrefixDir = PrefixDir.str();
+void RedirectingFileSystem::setOverlayFileDir(StringRef Dir) {
+  OverlayFileDir = Dir.str();
 }
 
-StringRef RedirectingFileSystem::getExternalContentsPrefixDir() const {
-  return ExternalContentsPrefixDir;
+StringRef RedirectingFileSystem::getOverlayFileDir() const {
+  return OverlayFileDir;
 }
 
 void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
@@ -1621,6 +1621,20 @@
 return None;
   }
 
+  Optional
+  parseRootRelativeKind(yaml::Node *N) {
+SmallString<12> Storage;
+StringRef Value;
+if (!parseScalarString(N, Value, Storage))
+  return None;
+if (Value.equals_insensitive("cwd")) {
+  return RedirectingFileSystem::RootRelativeKind::CWD;
+} else if (Value.equals_insensitive("overlay-dir")) {
+  return RedirectingFileSystem::RootRelativeKind::OverlayDir;
+}
+return None;
+  }
+
   struct KeyStatus {
 bool Required;
 bool Seen = false;
@@ -1828,7 +

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:660
 ///   'use-external-names': 
+///   'root-relative': 
 ///   'overlay-relative': 

haowei wrote:
> bnbarham wrote:
> > phosek wrote:
> > > Could we make this just a boolean akin to `overlay-relative` since there 
> > > are only two options (default to `false`)?
> > I personally prefer being explicit here, `overlay-relative` is fairly 
> > confusing as it is.
> > 
> > `overlay-relative` isn't about allowing relative paths, but instead means 
> > that *all* external paths should be prefixed with the directory of the 
> > overlay. To put another way, external paths can be relative whether this is 
> > true/false, `overlay-relative` just *always* prepends the overlay path.
> > 
> > Could you add a comment to make it clear that this has no interaction with 
> > `overlay-relative`? If you want to add a comment to `overlay-relative` with 
> > something like the above that would also be appreciated :)
> I also think `overlay-relative` is a bit misleading and it should be an enum 
> instead of a boolean option. And it should only work if the external paths 
> are relative instead of blindly prepend the overlay dir to every external 
> paths. But changing this will be a breaking change so I prefer to avoid it in 
> this patch.
> 
> I updated the descriptions.
FWIW it was added explicitly to always prepend, it's used for reproducers where 
the overlay may already have absolute paths. I was just trying to explain what 
`overlay-relative` does.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1903
+assert(!FullPath.empty() && "YAML file directory must exist");
+sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+Name = canonicalize(Name);

haowei wrote:
> bnbarham wrote:
> > IMO both this and CWD should be using the base FS instead. VFS didn't have 
> > a CWD previously, but now that it does it doesn't really make sense to use 
> > the process wide CWD. Especially since `-working-directory` doesn't change 
> > it.
> Could you clarify a bit more about the "base FS" please? I am still quite new 
> to the LLVM VFS system.  Which API should I use to get the appropriate 
> working directory instead of the the process wide CWD?
> 
> I avoided changing the default behavior (relative to the process's current 
> working directory) as I am a bit concerned breaking other people's use cases. 
> 
> I looked up a bit, the value "--working-directory" will be writen into 
> "VFS->setCurrentWorkingDirectory()" Do you think it is a better idea to use 
> this value instead of the process current working directory? Though it would 
> still be a behavior change for users rely on process current working 
> directories. 
> Could you clarify a bit more about the "base FS" please?

By "base FS" I just meant the filesystem that's passed down when creating the 
RedirectingFileSystem, ie. `ExternalFS` in `getVFSFromYAML`.

> Which API should I use to get the appropriate working directory instead of 
> the the process wide CWD?

`ExternalFS->getCurrentWorkingDirectory()` and `ExternalFS->makeAbsolute` is 
what I'd expect to be used here.

> Do you think it is a better idea to use this value instead of the process 
> current working directory? Though it would still be a behavior change for 
> users rely on process current working directories.

Yes, I think it's surprising that it currently *isn't* using it. It's just a 
hold over from when the VFS didn't have the concept of CWD. I'd be happy with 
accepting this patch as is, with a separate patch after that changes to using 
the VFS CWD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei added inline comments.



Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',

bnbarham wrote:
> bnbarham wrote:
> > I'd prefer a test without `overlay-relative` set to make it clear they 
> > don't depend on each other.
> There's also unit tests in `llvm/unittests/Support/VirtualFileSystemTest.cpp` 
> that you could add to.
I need some time to patch this unit test for testing the new option.
Existing functions in this unit test file does not support defining the 
YAMLFilePath.

I will update this patch when I finish.



Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',

haowei wrote:
> bnbarham wrote:
> > bnbarham wrote:
> > > I'd prefer a test without `overlay-relative` set to make it clear they 
> > > don't depend on each other.
> > There's also unit tests in 
> > `llvm/unittests/Support/VirtualFileSystemTest.cpp` that you could add to.
> I need some time to patch this unit test for testing the new option.
> Existing functions in this unit test file does not support defining the 
> YAMLFilePath.
> 
> I will update this patch when I finish.
I updated the yaml file and add unit tests with overlay-relative set to true 
and false.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:660
 ///   'use-external-names': 
+///   'root-relative': 
 ///   'overlay-relative': 

bnbarham wrote:
> phosek wrote:
> > Could we make this just a boolean akin to `overlay-relative` since there 
> > are only two options (default to `false`)?
> I personally prefer being explicit here, `overlay-relative` is fairly 
> confusing as it is.
> 
> `overlay-relative` isn't about allowing relative paths, but instead means 
> that *all* external paths should be prefixed with the directory of the 
> overlay. To put another way, external paths can be relative whether this is 
> true/false, `overlay-relative` just *always* prepends the overlay path.
> 
> Could you add a comment to make it clear that this has no interaction with 
> `overlay-relative`? If you want to add a comment to `overlay-relative` with 
> something like the above that would also be appreciated :)
I also think `overlay-relative` is a bit misleading and it should be an enum 
instead of a boolean option. And it should only work if the external paths are 
relative instead of blindly prepend the overlay dir to every external paths. 
But changing this will be a breaking change so I prefer to avoid it in this 
patch.

I updated the descriptions.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:752
+  enum class RootRelativeKind {
+/// The roots are relative to the current working directory.
+CWD,

bnbarham wrote:
> `to the current working directory when the overlay is created.` is maybe a 
> little clearer to me
Added into the comments.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:755
+/// The roots are relative to the directory where the YAML file locates.
+YAMLDir
+  };

bnbarham wrote:
> Any thoughts on something like `OverlayDir` instead?
I think OverlayDir is better.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1903
+assert(!FullPath.empty() && "YAML file directory must exist");
+sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+Name = canonicalize(Name);

bnbarham wrote:
> IMO both this and CWD should be using the base FS instead. VFS didn't have a 
> CWD previously, but now that it does it doesn't really make sense to use the 
> process wide CWD. Especially since `-working-directory` doesn't change it.
Could you clarify a bit more about the "base FS" please? I am still quite new 
to the LLVM VFS system.  Which API should I use to get the appropriate working 
directory instead of the the process wide CWD?

I avoided changing the default behavior (relative to the process's current 
working directory) as I am a bit concerned breaking other people's use cases. 

I looked up a bit, the value "--working-directory" will be writen into 
"VFS->setCurrentWorkingDirectory()" Do you think it is a better idea to use 
this value instead of the process current working directory? Though it would 
still be a behavior change for users rely on process current working 
directories. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 474115.
haowei marked 2 inline comments as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1482,12 +1482,12 @@
   return Combined;
 }
 
-void RedirectingFileSystem::setExternalContentsPrefixDir(StringRef PrefixDir) {
-  ExternalContentsPrefixDir = PrefixDir.str();
+void RedirectingFileSystem::setOverlayFileDir(StringRef Dir) {
+  OverlayFileDir = Dir.str();
 }
 
-StringRef RedirectingFileSystem::getExternalContentsPrefixDir() const {
-  return ExternalContentsPrefixDir;
+StringRef RedirectingFileSystem::getOverlayFileDir() const {
+  return OverlayFileDir;
 }
 
 void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
@@ -1621,6 +1621,20 @@
 return None;
   }
 
+  Optional
+  parseRootRelativeKind(yaml::Node *N) {
+SmallString<12> Storage;
+StringRef Value;
+if (!parseScalarString(N, Value, Storage))
+  return None;
+if (Value.equals_insensitive("cwd")) {
+  return RedirectingFileSystem::RootRelativeKind::CWD;
+} else if (Value.equals_insensitive("overlay-dir")) {
+  return RedirectingFileSystem::RootRelativeKind::OverlayDir;
+}
+return None;
+  }
+
   struct KeyStatus {
 bool Required;
 bool Seen = false;
@@ -1828,7 +1842,7 @@
 
 SmallString<256> FullPath;
 if (FS->IsRelativeOverlay) {
-  FullPath = FS->getExternalContentsPrefixDir();
+  FullPath = FS->getOverlayFileDir();
   assert(!FullPath.empty() &&
  "External contents prefix directory must exist");
   llvm::sys::path::append(FullPath, Value);
@@ -1884,6 +1898,15 @@
   } else if (sys::path::is_absolute(Name,
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
+  } else if (FS->RootRelative ==
+ RedirectingFileSystem::RootRelativeKind::OverlayDir) {
+SmallString<256> FullPath = FS->getOverlayFileDir();
+assert(!FullPath.empty() && "Overlay file directory must exist");
+sys::fs::make_absolute(FS->getOverlayFileDir(), Name);
+Name = canonicalize(Name);
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   } else {
 // Relative VFS root entries are made absolute to the current working
 // directory, then we can determine the path style from that.
@@ -1962,6 +1985,7 @@
 KeyStatusPair("version", true),
 KeyStatusPair("case-sensitive", false),
 KeyStatusPair("use-external-names", false),
+KeyStatusPair("root-relative", false),
 KeyStatusPair("overlay-relative", false),
 KeyStatusPair("fallthrough", false),
 KeyStatusPair("redirecting-with", false),
@@ -2051,6 +2075,13 @@
   error(I.getValue(), "expected valid redirect kind");
   return false;
 }
+  } else if (Key == "root-relative") {
+if (auto Kind = parseRootRelativeKind(I.getValue())) {
+  FS->RootRelative = *Kind;
+} else {
+  error(I.getValue(), "expected valid root-relative kind");
+  return false;
+}
   } else {
 llvm_unreachable("key missing from Keys");
   }
@@ -2100,13 +2131,13 @@
 // Example:
 //-ivfsoverlay dummy.cache/vfs/vfs.yaml
 // yields:
-//  FS->ExternalContentsPrefixDir => //dummy.cache/vfs
+//  FS->YAMLFileDir => //dummy.cache/vfs
 //
 SmallString<256> OverlayAbsDir = sys::path::parent_path(YAMLFilePath);
 std::error_code EC = llvm::sys::fs::make_absolute(OverlayAbsDir);
 assert(!EC && "Overlay dir final path must be absolute");
 (void)EC;
-FS->setExternalContentsPrefixDir(OverlayAbsDir);
+FS->setOverlayFileDir(OverlayAbsDir);
   }
 
   if (!P.parse(Root, FS.get()))
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -651,17 +651,28 @@
 /// \endverbatim
 ///
 /// The roots may be absolute or relative. If relative they will be made
-/// absolute against the current working directory.
+/// absolute against either current working directory or the directory where
+/// the Overlay YAML file is located, depending on the 'root-relative'
+/// configuration.
 ///
 /// All configuration options are op

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',

bnbarham wrote:
> I'd prefer a test without `overlay-relative` set to make it clear they don't 
> depend on each other.
There's also unit tests in `llvm/unittests/Support/VirtualFileSystemTest.cpp` 
that you could add to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

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

dexonsmith added a comment.

In D137473#3915497 , @bnbarham wrote:

> This seems reasonable to me in general. @dexonsmith in case you have any 
> thoughts.

SGTM! (I haven't reviewed in detail but I figure @bnbarham is on it...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a subscriber: dexonsmith.
bnbarham added a comment.

This seems reasonable to me in general. @dexonsmith in case you have any 
thoughts.




Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4
+  'case-sensitive': false,
+  'overlay-relative': true,
+  'root-relative': 'yaml-dir',

I'd prefer a test without `overlay-relative` set to make it clear they don't 
depend on each other.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:660
 ///   'use-external-names': 
+///   'root-relative': 
 ///   'overlay-relative': 

phosek wrote:
> Could we make this just a boolean akin to `overlay-relative` since there are 
> only two options (default to `false`)?
I personally prefer being explicit here, `overlay-relative` is fairly confusing 
as it is.

`overlay-relative` isn't about allowing relative paths, but instead means that 
*all* external paths should be prefixed with the directory of the overlay. To 
put another way, external paths can be relative whether this is true/false, 
`overlay-relative` just *always* prepends the overlay path.

Could you add a comment to make it clear that this has no interaction with 
`overlay-relative`? If you want to add a comment to `overlay-relative` with 
something like the above that would also be appreciated :)



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:752
+  enum class RootRelativeKind {
+/// The roots are relative to the current working directory.
+CWD,

`to the current working directory when the overlay is created.` is maybe a 
little clearer to me



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:755
+/// The roots are relative to the directory where the YAML file locates.
+YAMLDir
+  };

Any thoughts on something like `OverlayDir` instead?



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:926
+  /// is set. This will also be prefixed to each 'roots->name' if RootRelative
+  /// is set to RootRelativeKind::YAMLDir.
+  std::string YAMLFileDir;

*and the path is relative*



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1903
+assert(!FullPath.empty() && "YAML file directory must exist");
+sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+Name = canonicalize(Name);

IMO both this and CWD should be using the base FS instead. VFS didn't have a 
CWD previously, but now that it does it doesn't really make sense to use the 
process wide CWD. Especially since `-working-directory` doesn't change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.



Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:660
 ///   'use-external-names': 
+///   'root-relative': 
 ///   'overlay-relative': 

Could we make this just a boolean akin to `overlay-relative` since there are 
only two options (default to `false`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei added reviewers: bnbarham, keith.
haowei added a comment.

Please take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473

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

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

[Haowei Wu via Phabricator via cfe-commits]

haowei created this revision.
haowei added reviewers: phosek, abrachet.
Herald added a subscriber: hiraditya.
Herald added a project: All.
haowei requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

VFS overlay file allow using relative paths in root->name paths (root directory 
of the virtual file) and external-contents (the actual files). While the 
'external-contents' could be configured to relative to directory of the YAML 
file (change https://reviews.llvm.org/D17457), before this change, the root 
paths could only be relative to the current working directory.

This patch add the "root-relative" to the YAML file format. When this option is 
set to "yaml-dir" the root->name will be prepend by the YAML file directory. 
This option is helpful when compiling on case sensitive file systems when cross 
compiling to Windows as we can create a vfsoverlay YAML file for the Windows 
libraries without using absolute paths. Related change: 
https://reviews.llvm.org/D125800


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137473

Files:
  clang/test/VFS/Inputs/root-relative-overlay.yaml
  clang/test/VFS/relative-path.c
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1482,13 +1482,11 @@
   return Combined;
 }
 
-void RedirectingFileSystem::setExternalContentsPrefixDir(StringRef PrefixDir) {
-  ExternalContentsPrefixDir = PrefixDir.str();
+void RedirectingFileSystem::setYAMLFileDir(StringRef Dir) {
+  YAMLFileDir = Dir.str();
 }
 
-StringRef RedirectingFileSystem::getExternalContentsPrefixDir() const {
-  return ExternalContentsPrefixDir;
-}
+StringRef RedirectingFileSystem::getYAMLFileDir() const { return YAMLFileDir; }
 
 void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
   if (Fallthrough) {
@@ -1621,6 +1619,20 @@
 return None;
   }
 
+  Optional
+  parseRootRelativeKind(yaml::Node *N) {
+SmallString<9> Storage;
+StringRef Value;
+if (!parseScalarString(N, Value, Storage))
+  return None;
+if (Value.equals_insensitive("cwd")) {
+  return RedirectingFileSystem::RootRelativeKind::CWD;
+} else if (Value.equals_insensitive("yaml-dir")) {
+  return RedirectingFileSystem::RootRelativeKind::YAMLDir;
+}
+return None;
+  }
+
   struct KeyStatus {
 bool Required;
 bool Seen = false;
@@ -1828,7 +1840,7 @@
 
 SmallString<256> FullPath;
 if (FS->IsRelativeOverlay) {
-  FullPath = FS->getExternalContentsPrefixDir();
+  FullPath = FS->getYAMLFileDir();
   assert(!FullPath.empty() &&
  "External contents prefix directory must exist");
   llvm::sys::path::append(FullPath, Value);
@@ -1884,6 +1896,15 @@
   } else if (sys::path::is_absolute(Name,
 sys::path::Style::windows_backslash)) {
 path_style = sys::path::Style::windows_backslash;
+  } else if (FS->RootRelative ==
+ RedirectingFileSystem::RootRelativeKind::YAMLDir) {
+SmallString<256> FullPath = FS->getYAMLFileDir();
+assert(!FullPath.empty() && "YAML file directory must exist");
+sys::fs::make_absolute(FS->getYAMLFileDir(), Name);
+Name = canonicalize(Name);
+path_style = sys::path::is_absolute(Name, sys::path::Style::posix)
+ ? sys::path::Style::posix
+ : sys::path::Style::windows_backslash;
   } else {
 // Relative VFS root entries are made absolute to the current working
 // directory, then we can determine the path style from that.
@@ -1962,6 +1983,7 @@
 KeyStatusPair("version", true),
 KeyStatusPair("case-sensitive", false),
 KeyStatusPair("use-external-names", false),
+KeyStatusPair("root-relative", false),
 KeyStatusPair("overlay-relative", false),
 KeyStatusPair("fallthrough", false),
 KeyStatusPair("redirecting-with", false),
@@ -2051,6 +2073,13 @@
   error(I.getValue(), "expected valid redirect kind");
   return false;
 }
+  } else if (Key == "root-relative") {
+if (auto Kind = parseRootRelativeKind(I.getValue())) {
+  FS->RootRelative = *Kind;
+} else {
+  error(I.getValue(), "expected valid root-relative kind");
+  return false;
+}
   } else {
 llvm_unreachable("key missing from Keys");
   }
@@ -2100,13 +2129,13 @@
 // Example:
 //-ivfsoverlay dummy.cache/vfs/vfs.yaml
 // yields:
-//  FS->ExternalContentsPrefixDir => //dummy.cache/vfs
+//  FS->YAMLFileDir => //dummy.cache/vfs
 //
 SmallString<256> OverlayAbsDir = sys::path::parent_path(YAMLFilePath);