[PATCH] D70701: Fix more VFS tests on Windows

Thu Jan 21 17:36:00 PST 2021

 I didn't design VFS.  I just fixed a bunch of portability bugs that
prevented us from running most of the VFS tests on Windows.  If I were
designing it, I (hope) I wouldn't have done it this way.  But the horse is
already out of the barn.

Here's my background with VFS (and, specifically, the redirecting
filesystem):

VFS is used in many tests to map a path in an arbitrary (usually Posix)
style to another path, possibly in a different filesystem.  This allows the
test to be platform agnostic.  For example, the test can refer to
`/foo/bar.h` if it uses the redirecting filesystem to map `/foo` to an
actual directory on the host.  If it's a Windows machine, that target might
be something like `C:\foo`, in which case the actual file would be
`C:\foo\bar.h`, which LLVM thinks of as `C:\foo/bar.h`.  That's inherently
a path with hybrid style.  There are other cases, too, e.g., where the host
is Windows, but the target filesystem is an LLVM in-memory one (which uses
only Posix style).

When I first tried to tackle the portability bugs, I tried various
normalization/canonicalization strategies, but always encountered a
blocker.  That's when rnk pointed out to me that clang generally doesn't do
any path normalization; it just treats paths as strings that can be
concatenated.  With that in mind, I tried accepting the fact that hybrid
path styles are a fact of life in VFS, and suddenly nearly all of the
portability problems became relatively easy to solve.

Note that lots of LLVM and Clang tests were using VFS, but the VFS tests
themselves couldn't run on Windows.  All those tests were built upon
functionality that wasn't being tested.

I think that we probably could do something simpler, but it would force a
breaking change in the design of the redirecting filesystem.  The most
obvious victim of that break would be various LLVM and clang tests that
exclusively use Posix-style paths and rely on VFS to make it work on
non-Posix OSes.  I'm not sure how significant the break would be for others.

Looking specifically at your proposal:

> - 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?

- 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.

I don't remember the details, but canonicalizing the target paths caused
problems.  Do we need to be careful about multiple redirections (e.g.,
`/foo` directs to `/zerz` which directs to `C:\bumble`)?  I seem to recall
there was a good reason why the redirecting filesystem waits to the last
moment to convert a virtual path to an actual host path.

> - The nodes in-memory all use native paths so the non-native style isn't
needed after construction is complete.

I'm guessing that would affect how paths are displayed (e.g., in diagnostic
messages).  At a minimum, we'd have to fix some tests.  I don't know all
the contexts this might occur and how that might affect things.  For
example, paths may be written into debug info metadata.

- `makeAbsolute()` doesn't need to be overridden / special.

Honestly, I'm not sure we have a good definition of what `makeAbsolute`
should do.  Sure, on Posix, it's well understood.  But is `\foo` an
absolute path on Windows?  Is `D:bar.h` an absolute path on Windows?  If
not, how should those be made absolute?  LLVM assumes that there's a well
defined mapping between Posix filesystem concepts and the host filesystem.
But I haven't seen any documentation on how a Posix->Windows mapping should
work (let alone the inverse), and I certainly don't have an intuitive
understanding of how that mapping should work.

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.

I'm not opposed to making this better, but I don't think I understand your
proposal well enough to discuss it in detail.  I'm pretty sure anything
that eliminates hybrid paths is going to cause some breaking changes.  That
might be as simple as fixing up a bunch of tests, but it might have wider
impact.

Adrian.

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

> dexonsmith added a comment.
>
> @rnk / @amccarth, I've been looking at the history of `makeAbsolute` being
> virtual, since it's a bit awkward that `RedirectingFileSystem` behaves
> differently from the others. I'm hoping you can give me a bit more context.
>
> I'm wondering about an alternative implementation where:
>
> - The path style is detected when reading the YAML file (as now).
> - Paths in the YAML file are canonicalized to native at parse time.
> - The nodes in-memory all use native paths so the non-native style isn't
> needed after construction is complete.
> - `makeAbsolute()` doesn't need to be overridden / special.
>
> Was this considered? If so, why was it rejected? (If it doesn't work, why
> not?)
>
> If we could limit the scope the special version of `makeAbsolute()` to
> "construction time" it would simplify the mental model. As it stands it's a
> bit difficult to reason about `makeAbsolute`, and whether it's safe/correct
> to send a `makeAbsolute`-d path into `ExternalFS`.
>
>
> 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/20210121/75ec7de6/attachment.html>