[PATCH] D74488: [VFS] Fix vfsoverlay assertion due to RedirectingFileSystem path handling.

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

I'll look at this more later today, but here are my initial thoughts.

> ClangScanDeps/vfsoverlay test can fail on Windows

"can fail"?  Are you saying the results are flaky?  I haven't seen this test fail.

I never saw a situation where an external path could start with `C:/` rather than `C:\`.  The keys for VFS are sometimes weird hybrids of Posix and Windows styles, but I can't figure out how a VFS lookup would result in an absolute path with that kind of prefix.



================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1002
     style = (Path[n] == '/') ? llvm::sys::path::Style::posix
                              : llvm::sys::path::Style::windows;
 
----------------
I was hesitant to do it this way, but, for an arbitrary path, I didn't see a better alternative.  Maybe there's something smarter we can do here so that you don't have to special case the fix.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1694
+
   // Canonicalize path by removing ".", "..", "./", components. This is
   // a VFS request, do not bother about symlinks in the path components
----------------
Nit:  With the change below, this comment becomes slightly misleading.  remove_dots doesn't remove a leading "./".  Of course, there won't be one because the path is absolute.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1697
   // but canonicalize in order to perform the correct entry search.
-  Path = canonicalize(Path);
+  llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true, path_style);
   if (Path.empty())
----------------
I'm uneasy with this.  The intent of introducing canonicalize was to reduce the inconsistencies in several parts of this code:  some considered path style, others didn't; some removed certain dot patterns but not others.

I'd prefer it if we could fix canonicalize rather than to try to work around a special case at one call site.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1701
 
-  sys::path::const_iterator Start = sys::path::begin(Path);
+  sys::path::const_iterator Start = sys::path::begin(Path, path_style);
   sys::path::const_iterator End = sys::path::end(Path);
----------------
At one point, I, too, put the path_style here, but it has no tangible difference, so I thought it was best to leave it out.


================
Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:2237
+
+TEST_F(VFSFromYAMLTest, ForwardslashWindowsPath) {
+  IntrusiveRefCntPtr<DummyFileSystem> Lower(new DummyFileSystem());
----------------
This doesn't seem like a valid test.  It's one thing for some simplistic path code to append Windows and Posix path fragments together, resulting in a Windows-like path that's syntactically incorrect.  But it's another thing to feed it syntactically incorrect absolute paths.  That will always eventually lead to ambiguous situations where there is no correct option for the code.

I think the VFS YAML reader should reject yaml.  Specifically an absolute `external-contents` path with incorrect syntax.

[And, yes, it _is_ incorrect syntax on Windows.  Most WinAPIs will correct the slash direction during normalization, but Windows has local device paths (`\\?\`) that bypass normalization and UNC paths which require the correct separators.]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74488