[PATCH] D70701: Fix more VFS tests on Windows

[Adrian McCarthy via llvm-commits]

BTW, I hope I didn't come across as overly negative in my previous
response.  I'd love to see the situation improved.  I just don't know what
that would look like.

> One thing I see, but wasn't obvious from your description, is that the
mixed/hybrid separator behaviour only happens for `defined(_WIN_32)`.
> [copy of `is_separator` elided]

It's a runtime decision based on the specified style, not a compile-time
one based on _WIN_32.  If the caller of `is_separator` passes
`Style::windows` then it'll accept either `/` or `\\`, even if LLVM was
compiled and run on Linux or Mac.  I believe there's a VFS test that
redirects a Windows-style path to a Posix-style one (an in-memory file
system), and that test passes on both kinds of hosts.

But I get the gist of the point.  My feeling is that, unless we can
eliminate hybrid styles, Paths should support a Style::hybrid.  It would be
messy because more ambiguities about how to map things crop up.

> Honestly, I'm not sure we have a good definition of what makeAbsolute
> should do.

I should have said:  **I** don't have a good understanding of what
makeAbsolute should do.  Even saying it should prepend the current working
directory is an incomplete answer.  On Windows, a process can have multiple
current directories:  one for each drive.  And a process has one current
drive.  So the current working directory is the current directory for the
current drive.  A Windows path like "D:foo.txt" is a relative path.
Literally prepending the current working directory gives us
`C:\users\me\D:foo.txt`, which is syntactically wrong.  But even if we're
smart enough to fix up the syntax, we'd get `C:\users\me\foo.txt` or
`D:\users\me\foo.txt`, both of which would (likely) be wrong.  The way the
OS would resolve it is to look up the process's current directory for the
D: drive, and insert that into the missing bit.

Anyway, I look forward to any and all proposals to improve this situation.


On Thu, Jan 21, 2021 at 6:47 PM Duncan P. N. Exon Smith via Phabricator <
reviews at reviews.llvm.org> wrote:

> dexonsmith added a comment.
>
> Thanks for the quick and detailed response!
>
> Your explanation of hybrid behaviour on Windows was especially helpful, as
> I didn't fully understand how that worked before.
>
> One thing I see, but wasn't obvious from your description, is that the
> mixed/hybrid separator behaviour only happens for `defined(_WIN_32)`. E.g.,
> from Path.cpp:
>
>   bool is_separator(char value, Style style) {
>     if (value == '/')
>       return true;
>     if (real_style(style) == Style::windows)
>       return value == '\\';
>     return false;
>   }
>
>
>
> In D70701#2514143 <https://reviews.llvm.org/D70701#2514143>, @amccarth
> wrote:
>
> >> - The path style is detected when reading the YAML file (as now).
> >
> > Which path's style?  The virtual one that's going to be redirected or the
> > actual one it's redirected at?
>
> Both, but you've mostly convinced me not to go down this route.
>
> > - Paths in the YAML file are canonicalized to native at parse time.
> >
> > If we canonicalize the virtual path, VFS would no longer be valuable for
> > creating platform-agnostic tests.
>
> This is a good point I hadn't considered.
>
> > In LLDB, we have the additional wrinkle of remote debugging, where the
> > debugger may be running on a Windows machine while the program being
> > debugged is running on a Linux box.  You always have to know whether a
> path
> > will be used on the debugger host or the debuggee host.  And there are
> > similar concerns for post-mortem debugging from a crash collected on a
> > different type of host.
>
> Ah, interesting.
>
> > Honestly, I'm not sure we have a good definition of what makeAbsolute
> > should do.
>
> Perhaps the name isn't ideal --
> `prependRelativePathsWithCurrentWorkingDirectory()` would be more precise
> -- but otherwise I'm not sure I fully agree. Regardless, I acknowledge your
> point that the two worlds are awkwardly different.
>
> I'm going to think about other options; thanks again for your feedback. I
> am still leaning toward `FileSystem::makeAbsolute()` not being virtual /
> overridden, but I have a better idea of how to approach that. One idea is
> for the `RedirectingFileSystem` to keep track of where different styles
> were used when parsing.... I'm not sure if that'll pan out though.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D70701/new/
>
> https://reviews.llvm.org/D70701
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210122/0368f7e6/attachment.html>