[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, they should not be treated as path separators. This is how it is currently implemented in LLVM path library and VFS library didn't perform any path separator conversion as well. My patch didn't change this behavior.

Do you think we still need to add additional tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137473