[PATCH] D70701: Fix more VFS tests on Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078
+std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
+  if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) ||
+      llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::windows))
+    return {};
----------------
amccarth wrote:
> amccarth wrote:
> > rnk wrote:
> > > I think Posix-style paths are considered absolute, even on Windows. The opposite isn't true, a path with a drive letter is considered to be relative if the default path style is Posix. But, I don't think that matters. We only end up in this mixed path style situation on Windows.
> > > 
> > > Does this change end up being necessary? I would expect the existing implementation of makeAbsolute to do the right thing on Windows as is. I think the other change seems to be what matters.
> > Yes, it's necessary.  The Posix-style path `\tests` is not considered absolute on Windows.  Thus the original makeAboslute would merge it with the current working directory, which gives it a drive letter, like `D:\tests\`.  The drive letter component then causes comparisons to fail.
> To make sure I wasn't misremembering or hallucinating, I double-checked the behavior here:  Posix-style paths like `\tests` are not considered absolute paths in Windows-style.
I see, I agree, the platforms diverge here:
  bool rootDir = has_root_directory(p, style);
  bool rootName =
      (real_style(style) != Style::windows) || has_root_name(p, style);

  return rootDir && rootName;

So, on Windows rootDir is true and rootName is false.

I still wonder if this behavior shouldn't be pushed down into the base class. If I pass the path `\test` to the real `FileSystem::makeAbsolute` on Windows, should that prepend the CWD, or should it leave the path alone?


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
 
-    if (IsRootEntry && !sys::path::is_absolute(Name)) {
-      assert(NameValueNode && "Name presence should be checked earlier");
----------------
amccarth wrote:
> rnk wrote:
> > Is there a way to unit test this? I see some existing tests in llvm/unittests/Support/VirtualFileSystemTest.cpp.
> > 
> > I looked at the yaml files in the VFS tests this fixes, and I see entries like this:
> >     { 'name': '/tests', 'type': 'directory', ... },
> >     { 'name': '/indirect-vfs-root-files', 'type': 'directory', ... },
> >     { 'name': 'C:/src/llvm-project/clang/test/VFS/Inputs/Broken.framework', 'type': 'directory', ... },
> >     { 'name': 'C:/src/llvm-project/build/tools/clang/test/VFS/Output/vfsroot-with-overlay.c.tmp/cache', 'type': 'directory', ... },
> > 
> > So it makes sense to me that we need to handle both kinds of absolute path.
> > Is there a way to unit test this?
> 
> What do you mean by "this"?  The `/tests` and `/indirect-vfs-root-files` entries in that yaml are the ones causing several tests to fail without this fix, so I take it that this is already being tested.  But perhaps you meant testing something more specific?
> What do you mean by "this"? 
I guess what I meant was, can you unit test the whole change in case there are behavior differences here not covered by the clang tests?


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1448-1449
+      // which style we have, and use it consistently.
+      if (sys::path::is_absolute(Name, sys::path::Style::posix)) {
+        path_style = sys::path::Style::posix;
+      } else if (sys::path::is_absolute(Name, sys::path::Style::windows)) {
----------------
amccarth wrote:
> amccarth wrote:
> > rnk wrote:
> > > I wonder if there's a simpler fix here. If the working directory is an absolute Windows-style path, we could prepend the drive letter of the working directory onto any absolute Posix style paths read from YAML files. That's somewhat consistent with what native Windows tools do. In cmd, you can run `cd \Windows`, and that will mean `C:\Windows` if the active driver letter is C. I think on Windows every drive has an active directory, but that's not part of the file system model.
> > I'm not seeing how this would be simpler.
> > 
> > I don't understand the analogy of your proposal to what the native Windows tools do.  When you say, `cd \Windows`, the `\Windows` is _not_ an absolute path.  It's relative to the current drive.
> > 
> > I could be wrong, but I don't think prepending the drive onto absolution Posix style paths read from YAML would work.  That would give us something like `D:/tests` (which is what was happening in makeAbsolute) and that won't match paths generated from non-YAML sources, which will still come out as `/tests/foo.h`.
> > 
> > > I think on Windows every drive has an active directory ....
> > 
> > Yep.  I think of it as "every drive has a _current_ directory" and each process has a current drive.  When you want the current working directory, you get the current directory of the current drive.
> > ... I don't think prepending the drive onto absolution Posix style paths read from YAML would work....
> 
> I misspoke.  The idea of prepending the drive onto absolute Posix-style paths read from the YAML probably could be made to work for the common cases like the ones in these tests.
> 
> But I still don't think that approach would simplify anything.
> 
> Furthermore, I think it could open a potential Pandora's box of weird corner cases.  For example, in a system with multiple drives, the current drive may not always be the "correct" one to use.  Slapping a drive letter onto a Posix-style path creates a Frankenstein hybrid that's neither Windows-style nor Posix-style.  Doing so because we know the subsequent code would then recognize it as an absolute path seems like a way to create an unnecessary coupling between the VFS YAML parser and the LLVM path support.
> 
> In my mind, the model here is that these roots can be in either style.  I prefer to let the code explicitly reflect that fact rather than trying to massage some of the paths into a form that happens to cause the right outcome.
The way in which I see it as being "simpler" is that all absolute paths would end up having drive letters on Windows. I was hoping that would avoid some corner cases that arise from having a virtual file system tree in memory where some files are rooted in a drive letter and some appear in non-drive letter top level directories from Posix paths.

In any case, I think the change you have here is definitely correct, and is a smaller change in behavior. The path component iterators below need to use the correct style.


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

https://reviews.llvm.org/D70701