[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth marked 3 inline comments as done.
amccarth 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 {};
----------------
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.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431
 
-    if (IsRootEntry && !sys::path::is_absolute(Name)) {
-      assert(NameValueNode && "Name presence should be checked earlier");
----------------
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?


================
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)) {
----------------
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.


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

https://reviews.llvm.org/D70701