Page MenuHomePhabricator

Enable most VFS tests on Windows
AbandonedPublic

Authored by amccarth on Oct 14 2019, 11:57 AM.

Details

Reviewers
rnk
Summary

Inconsistencies in when/if paths are cannoicalized prevented VFS tests from working on Windows. This fixes the primary cause and enables most of the tests (15/20).

Remaining VFS tests are still XFAILed on Windows because there are additional causes in some code paths. I plan to diagnose and correct those in a future patch.

Diff Detail

Event Timeline

amccarth created this revision.Oct 14 2019, 11:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2019, 11:57 AM

+VFS people, mostly just to let them know
@arphaman @Bigcheese @vsapsai

Code changes all look good to me, but I had a bunch of questions.

clang/lib/Lex/HeaderSearch.cpp
782–783

I'm surprised this just works. :) You ran the whole clang test suite, and nothing broke, right? I would've expected something to accidentally depend on this behavior.

clang/test/VFS/external-names.c
4–5

You made changes to this file, but it's still XFAILed. Is that intentional?

llvm/lib/Support/VirtualFileSystem.cpp
1619–1620

After reading the code for a while, I see that drive letters are handled as just another component in the path, so I guess this code all works without explicitly considering drive letters.

1839

I think this will ultimately produce paths that look like: \C:\foo\bar\baz. Ultimately, we loop over the components above and repeatedly call sys::path::append on them.

Clang only uses this function for collecting module dependency info, it seems. Maybe that's what the remaining failures are about?

amccarth marked 2 inline comments as done.Oct 25 2019, 9:47 AM

Somehow Phabricator failed to notify me that you'd already left comments. I even searched my inbox to see if I'd just missed the notification. Nope. Nothing. I came here today to ping you for the review and discovered you'd already done your part.

Anyway, I'll update the patch and ping again when that's done.

Thanks.

clang/lib/Lex/HeaderSearch.cpp
782–783

Yeah, I recall it all working before, but, double-checking today, I'm seeing some collateral damage. Investigating.

clang/test/VFS/external-names.c
4–5

The change removes one obstacle, but the test still fails on Windows because there's still an outstanding bug. If you like, I can hoist this change out until I have a patch to fix the outstanding issue.

llvm/lib/Support/VirtualFileSystem.cpp
1839

I didn't encounter anything like \C:\foo. Perhaps the (enabled) VFS tests all prime the roots so that never happens here. I'll see if I can add a test to uncover that problem.

And, yes, use of this function seems limited to reading VFS paths from the YAML, so its scope is pretty limited. At least some of the remaining problems have to do with the fact that, while the target paths can always be expressed in the host style, the key paths can be in either Posix or Windows format, which leads to confusion when you try to get answers from functions like sys::path::is_absolute where you have to pick the right style to get the right answer.

amccarth abandoned this revision.Nov 7 2019, 11:04 AM

I create a new review thread for my improved patch shortly. These changes were too wide-ranging.