<div dir="ltr">
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.<div><br></div><div>Here's my background with VFS (and, specifically, the redirecting filesystem):</div><div><br></div><div>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).</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Looking specifically at your proposal:<br></div><div><br></div><div>> - The path style is detected when reading the YAML file (as now).</div><div><br></div><div>Which path's style? The virtual one that's going to be redirected or the actual one it's redirected at?</div><div><br>- Paths in the YAML file are canonicalized to native at parse time.</div><div><br></div><div>If we canonicalize the virtual path, VFS would no longer be valuable for creating platform-agnostic tests.</div><div><br></div><div>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.</div><div><br>> - The nodes in-memory all use native paths so the non-native style isn't needed after construction is complete.</div><div><br></div><div>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.</div><div><br>- `makeAbsolute()` doesn't need to be overridden / special. <br></div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Adrian.</div><div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 21, 2021 at 4:26 PM Duncan P. N. Exon Smith via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">dexonsmith added a comment.<br>
<br>
@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.<br>
<br>
I'm wondering about an alternative implementation where:<br>
<br>
- The path style is detected when reading the YAML file (as now).<br>
- Paths in the YAML file are canonicalized to native at parse time.<br>
- The nodes in-memory all use native paths so the non-native style isn't needed after construction is complete.<br>
- `makeAbsolute()` doesn't need to be overridden / special.<br>
<br>
Was this considered? If so, why was it rejected? (If it doesn't work, why not?)<br>
<br>
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`.<br>
<br>
<br>
Repository:<br>
rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D70701/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D70701/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D70701" rel="noreferrer" target="_blank">https://reviews.llvm.org/D70701</a><br>
<br>
</blockquote></div>