Page MenuHomePhabricator

Allow setting the VFS to 'real' mode instead of default 'physical'
AbandonedPublic

Authored by jfb on Aug 8 2019, 5:14 PM.

Details

Summary

The motivation for 'physical' mode in D58169 was pretty sensible, but it has one unfortunate side-effect: working close to the maximum path size now causes failures when it used not to. We therefore want to be able to set the VFS mode for the driver. This requires moving where the driver's VFS is initialized because arguments aren't available in the Driver constructor. I was careful to put this initialization early enough that the VFS isn't used yet (it would segfault if it were, since it's not set).

rdar://problem/54103540

Event Timeline

jfb created this revision.Aug 8 2019, 5:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 5:14 PM
jfb marked an inline comment as done.Aug 8 2019, 5:17 PM
jfb added inline comments.
clang/test/Driver/vfsmode.py
4 ↗(On Diff #214265)

I'm not sure what the best way to test this on Windows would be, and without a machine handy I can't really test this :-(

arphaman added inline comments.
clang/lib/Driver/Driver.cpp
167

It might be nice to break here to exit early, unless you want to make Clang pick the last one?

JDevlieghere added inline comments.Aug 8 2019, 5:27 PM
clang/lib/Driver/Driver.cpp
172

Is this really necessary?

981

s/eatly/early/

clang/test/Driver/vfsmode.py
18 ↗(On Diff #214265)

Can we pass current here?

MaskRay added a subscriber: MaskRay.Aug 8 2019, 6:21 PM
jfb updated this revision to Diff 214291.Aug 8 2019, 9:36 PM
jfb marked 6 inline comments as done.
  • Address comments.
clang/lib/Driver/Driver.cpp
172

the Driver ctor takes a VFS parameter which some tools set (otherwise it defaults to nullptr), so if the command-line VFS isn't specified we want to leave whatever was passed in as-is. If the command line parameter is set the we'll obey that instead.

So yes I think it's the right thing to do.

clang/test/Driver/vfsmode.py
18 ↗(On Diff #214265)

Nice catch, I think this is required for correctness (in case the CWD's filesystem is different from /'s and they have different configurations).

jfb updated this revision to Diff 214292.Aug 8 2019, 9:38 PM
  • Undo whitespaces change.
JDevlieghere accepted this revision.Aug 8 2019, 10:06 PM

Two small suggestions for the test, but this change LGTM.

clang/test/Driver/vfsmode.py
33 ↗(On Diff #214292)

I like the Pythonic approach, but I wonder if new_name_len = min(target_len - len(absolute) - 1, name_max) wouldn't be easier to read.

48 ↗(On Diff #214292)

Should we add an assert not os.path.exists(absolute) for good measure?

This revision is now accepted and ready to land.Aug 8 2019, 10:06 PM
jfb updated this revision to Diff 214296.Aug 8 2019, 10:15 PM
jfb marked an inline comment as done.
  • Address more comments.
jfb marked an inline comment as done.Aug 8 2019, 10:15 PM
JDevlieghere added inline comments.Aug 8 2019, 10:18 PM
clang/lib/Driver/Driver.cpp
172

FWIW, I got confused by the indentation of the case element. Even though it's in the if-case it just acts like a label. While clever, I think for the sake of readability it might be worth just repeating this->VFS = llvm::vfs::createPhysicalFileSystem().release();.

jfb updated this revision to Diff 214298.Aug 8 2019, 10:23 PM
jfb marked an inline comment as done.
  • Address more comments.
jfb marked an inline comment as done.Aug 8 2019, 10:23 PM
jfb added inline comments.
clang/lib/Driver/Driver.cpp
172

Ah gotcha, updated.

JDevlieghere accepted this revision.Aug 8 2019, 10:44 PM

Thanks for bearing with me, JF!

labath added inline comments.Aug 8 2019, 11:45 PM
clang/test/Driver/vfsmode.py
4 ↗(On Diff #214265)

Unsupported might be actually correct here. I am not an expert on windows, but I have a vague recollection that there, the platform's maximum path limit applies to the final absolute path of the file, irrespective of how you happen to refer to that file.

Tagging D62271 and @Bigcheese as this was the change that switched the default in Driver. (My motivation for D58169 was thread-heavy programs like clangd).

(At this point it seems likely that MAX_PATH and working directory issues are going to be our greatest defenses against the robot uprising)

clang/include/clang/Driver/Options.td
2010

Referring to this as vfs/"use the virtual file system..." seems possibly misleading, as we're talking about how we're using the *real* filesystem (through the virtual interface)

2012

Mea culpa: naming these physical/real doesn't really explain either the implementation difference nor the observable effect.
(I thought this internal name didn't matter much and didn't want to rename getRealFilesystem...)

Can we avoid propagating these bad names further? Maybe instead something like -os-workdir or -workdir=os|virtual or similar?

jfb marked 2 inline comments as done.Aug 9 2019, 9:47 AM
jfb added inline comments.
clang/test/Driver/vfsmode.py
4 ↗(On Diff #214265)

This document says:

Because you cannot use the "\\?\" prefix with a relative path, relative paths are always limited to a total of MAX_PATH characters.

So it seems relative paths can contain up to MAX_PATH.

Though there are limitations that I don't honor in this test such as:

When using an API to create a directory, the specified path cannot be so long that you cannot append an 8.3 file name (that is, the directory name cannot exceed MAX_PATH minus 12).

And then there's longPathAware which removes restrictions if the machine and the application opt-in...

So I'm very happy to ignore this UNIX test on Windows.

jfb marked 4 inline comments as done.Aug 9 2019, 9:50 AM
jfb added inline comments.
clang/include/clang/Driver/Options.td
2010

🤷‍♂️

Let's discuss below.

2012

Maybe? What do other folks who actually tough VFS think? I'm happy to go either way, I was following what seemed to be the prevalent naming (and parroted the comments on those functions) but I understand that it might not have been naming that you thought would leave those function 🙂

jfb marked an inline comment as done.Aug 9 2019, 9:51 AM

Tagging D62271 and @Bigcheese as this was the change that switched the default in Driver. (My motivation for D58169 was thread-heavy programs like clangd).

(At this point it seems likely that MAX_PATH and working directory issues are going to be our greatest defenses against the robot uprising)

Thanks for the reference, I hadn't seen this commit! Michael, WDYT?

bruno accepted this revision.Aug 9 2019, 9:53 AM

LGTM

clang/test/Driver/vfsmode.py
47 ↗(On Diff #214298)

If you want to make this even more explicit, you can also do assert os.path.exists(file_relative_path), "Your assert message here"

jfb updated this revision to Diff 214397.Aug 9 2019, 10:03 AM
  • Document asserts.
jfb marked 2 inline comments as done.Aug 9 2019, 10:03 AM
jfb added inline comments.
clang/test/Driver/vfsmode.py
47 ↗(On Diff #214298)

I updated all the asserts.

This fix works, but we could also use openat to get around max path length issues. Windows also has an API that can be used similarly.

jfb marked an inline comment as done.Aug 9 2019, 10:31 AM

This fix works, but we could also use openat to get around max path length issues. Windows also has an API that can be used similarly.

Given the type of test we're seeing fail, I'm worried that any other approach would be too invasive.

JDevlieghere added inline comments.Aug 9 2019, 1:44 PM
clang/include/clang/Driver/Options.td
2012

Personally I like the -workdir=os|virtual because it better conveys the semantic meaning, rather than how it's implemented.

jfb updated this revision to Diff 214448.Aug 9 2019, 1:59 PM
jfb marked an inline comment as done.
  • Rename option.
jfb marked an inline comment as done.Aug 9 2019, 2:00 PM
jfb added inline comments.
clang/include/clang/Driver/Options.td
2012

I renamed it. I think it's better to abandon this revision and go with Michael's suggestion, though.

jfb updated this revision to Diff 214477.Aug 9 2019, 4:18 PM
  • Also update function name
jfb abandoned this revision.Aug 9 2019, 4:18 PM

Let's do the better fix that Michael suggested.