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

[jfb]

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

[jfb]

clang/test/Driver/vfsmode.py

5

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]

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]

clang/lib/Driver/Driver.cpp

172

Is this really necessary?

981

s/eatly/early/

clang/test/Driver/vfsmode.py

19

Can we pass current here?

[jfb]

• 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

19

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]

• Undo whitespaces change.

[JDevlieghere]

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

clang/test/Driver/vfsmode.py

34

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.

49

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

[jfb]

• Address more comments.

[JDevlieghere]

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]

• Address more comments.

[jfb]

clang/lib/Driver/Driver.cpp

172

Ah gotcha, updated.

[JDevlieghere]

Thanks for bearing with me, JF!

[labath]

clang/test/Driver/vfsmode.py

5

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.

[sammccall]

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

2009

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)

2011

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]

clang/test/Driver/vfsmode.py

5

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](https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#enable-long-paths-in-windows-10-version-1607-and-later) which removes restrictions if the machine and the application opt-in...

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

[jfb]

clang/include/clang/Driver/Options.td

2009

🤷‍♂️

Let's discuss below.

2011

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]

In D65986#1622447, @sammccall wrote:

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]

LGTM

clang/test/Driver/vfsmode.py

48

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]

• Document asserts.

[jfb]

clang/test/Driver/vfsmode.py

48

I updated all the asserts.

[Bigcheese]

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]

In D65986#1623283, @Bigcheese wrote:

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]

clang/include/clang/Driver/Options.td

2011

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

[jfb]

• Rename option.

[jfb]

clang/include/clang/Driver/Options.td

2011

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

[jfb]

• Also update function name

[jfb]

Let's do the better fix that Michael suggested.

clang/include/clang/Driver/Driver.h

public:
   /// argument vector. A null return value does not necessarily
   /// indicate an error condition, the diagnostics should be queried
   /// to determine if an error occurred.
   Compilation *BuildCompilation(ArrayRef<const char *> Args);
 
   /// @name Driver Steps
   /// @{
 
-  /// ParseDriverMode - Look for and handle the driver mode option in Args.
+  /// Look for and handle the VFS mode option in Args.
+  void ParseVFSMode(ArrayRef<const char *> Args);
+
+  /// Look for and handle the driver mode option in Args.
   void ParseDriverMode(StringRef ProgramName, ArrayRef<const char *> Args);
 
-  /// ParseArgStrings - Parse the given list of strings into an
-  /// ArgList.
+  /// Parse the given list of strings into an ArgList.
   llvm::opt::InputArgList ParseArgStrings(ArrayRef<const char *> Args,
                                           bool IsClCompatMode,
                                           bool &ContainsError);
 
   /// BuildInputs - Construct the list of inputs and their types from
   /// the given arguments.
   ///
   /// \param TC - The default host tool chain.

clang/include/clang/Driver/Options.td

def iwithprefixbefore : JoinedOrSeparate<["-"], "iwithprefixbefore">, Group<clang_i_Group>,
   HelpText<"Set directory to include search path with prefix">, MetaVarName<"<dir>">,
   Flags<[CC1Option]>;
 def iwithprefix : JoinedOrSeparate<["-"], "iwithprefix">, Group<clang_i_Group>, Flags<[CC1Option]>,
   HelpText<"Set directory to SYSTEM include search path with prefix">, MetaVarName<"<dir>">;
 def iwithsysroot : JoinedOrSeparate<["-"], "iwithsysroot">, Group<clang_i_Group>,
   HelpText<"Add directory to SYSTEM include search path, "
            "absolute paths are relative to -isysroot">, MetaVarName<"<directory>">,
   Flags<[CC1Option]>;
-def ivfsoverlay : JoinedOrSeparate<["-"], "ivfsoverlay">, Group<clang_i_Group>, Flags<[CC1Option]>,
-  HelpText<"Overlay the virtual filesystem described by file over the real file system">;
+def ivfsoverlay : JoinedOrSeparate<["-"], "ivfsoverlay">, Group<clang_i_Group>, Flags<[CC1Option]>, HelpText<"Overlay the virtual filesystem described by file over the real file system">;
+def ivfsmode : Joined<["-"], "ivfsmode=">, Group<clang_i_Group>, Flags<[CC1Option, HelpHidden]>,
+  HelpText<"Use the virtual file system in 'real' mode, or 'physical' mode. In 'real' mode the working directory is linked to the process' working directory. In 'physical' mode it has its own working directory, independent of (but initially equal to) that of the process.">,
+  Values<"real,physical">;
 def imultilib : Separate<["-"], "imultilib">, Group<gfortran_Group>;
 def keep__private__externs : Flag<["-"], "keep_private_externs">;
 def l : JoinedOrSeparate<["-"], "l">, Flags<[LinkerInput, RenderJoined]>,
         Group<Link_Group>;
 def lazy__framework : Separate<["-"], "lazy_framework">, Flags<[LinkerInput]>;
 def lazy__library : Separate<["-"], "lazy_library">, Flags<[LinkerInput]>;
 def mlittle_endian : Flag<["-"], "mlittle-endian">, Flags<[DriverOption]>;
 def EL : Flag<["-"], "EL">, Alias<mlittle_endian>;

clang/lib/Driver/Driver.cpp

: Opts(createDriverOptTable()), Diags(Diags), VFS(std::move(VFS)),
       SysRoot(DEFAULT_SYSROOT), DriverTitle("clang LLVM compiler"),
       CCPrintOptionsFilename(nullptr), CCPrintHeadersFilename(nullptr),
       CCLogDiagnosticsFilename(nullptr), CCCPrintBindings(false),
       CCPrintOptions(false), CCPrintHeaders(false), CCLogDiagnostics(false),
       CCGenDiagnostics(false), TargetTriple(TargetTriple),
       CCCGenericGCCName(""), Saver(Alloc), CheckInputsExist(true),
       GenReproducer(false), SuppressMissingInputWarning(false) {
 
-  // Provide a sane fallback if no VFS is specified.
-  if (!this->VFS)
-    this->VFS = llvm::vfs::createPhysicalFileSystem().release();
-
   Name = llvm::sys::path::filename(ClangExecutable);
   Dir = llvm::sys::path::parent_path(ClangExecutable);
   InstalledDir = Dir; // Provide a sensible default installed dir.
 
 #if defined(CLANG_CONFIG_FILE_SYSTEM_DIR)
   SystemConfigDir = CLANG_CONFIG_FILE_SYSTEM_DIR;
 #endif
 #if defined(CLANG_CONFIG_FILE_USER_DIR)
   UserConfigDir = CLANG_CONFIG_FILE_USER_DIR;
 #endif
 
   // Compute the path to the resource directory.
   ResourceDir = GetResourcesPath(ClangExecutable, CLANG_RESOURCE_DIR);
 }
 
+void Driver::ParseVFSMode(ArrayRef<const char *> Args) {
+  const std::string OptionPrefix =
+      getOpts().getOption(options::OPT_ivfsmode).getPrefixedName();
+  enum class VFSMode { Unknown, Real, Physical } Mode = VFSMode::Unknown;
+  for (const char *ArgPtr : Args) {
+    if (!ArgPtr)
+      continue;
+    const StringRef Arg = ArgPtr;
+    if (!Arg.startswith(OptionPrefix))
+      continue;
+    const StringRef Value = Arg.drop_front(OptionPrefix.size());
+    VFSMode M = llvm::StringSwitch<VFSMode>(Value)
+                    .Case("real", VFSMode::Real)
+                    .Case("physical", VFSMode::Physical)
+                    .Default(VFSMode::Unknown);
+    if (M == VFSMode::Unknown) {
+      Diag(diag::err_drv_unsupported_option_argument) << OptionPrefix << Value;
+      break;
+    }
+    Mode = M;
+  }
+
+  switch (Mode) {
+  case VFSMode::Unknown:
+    if (!this->VFS) {
+      LLVM_FALLTHROUGH;
+    case VFSMode::Physical:
+      this->VFS = llvm::vfs::createPhysicalFileSystem().release();
+    }
+    break;
+  case VFSMode::Real:
+    this->VFS = llvm::vfs::getRealFileSystem().get();
+    break;
+  }
+}
+
 void Driver::ParseDriverMode(StringRef ProgramName,
                              ArrayRef<const char *> Args) {
   if (ClangNameParts.isEmpty())
     ClangNameParts = ToolChain::getTargetAndModeFromProgramName(ProgramName);
   setDriverModeFromOption(ClangNameParts.DriverMode);
 
   for (const char *ArgPtr : Args) {
     // Ignore nullptrs, they are the response file's EOL markers.
if (Optional<std::string> CompilerPathValue =
     while (!CompilerPath.empty()) {
       std::pair<StringRef, StringRef> Split =
           CompilerPath.split(llvm::sys::EnvPathSeparator);
       PrefixDirs.push_back(Split.first);
       CompilerPath = Split.second;
     }
   }
 
+  // We might try accessing the VFS fairly early, so make sure we have the
+  // right one.
+  ParseVFSMode(ArgList.slice(1));
+
   // We look for the driver mode option early, because the mode can affect
   // how other options are parsed.
   ParseDriverMode(ClangExecutable, ArgList.slice(1));
 
   // FIXME: What are we going to do with -V and -b?
 
   // Arguments specified in command line.
   bool ContainsError;

clang/test/Driver/vfsmode.py

+# Check that we can change the VFS mode between 'real' and 'physical', which
+# affects the maximum path length that clang can deal with.
+
+# UNSUPPORTED: system-windows
+
+# RUN: rm -rf xxx*
+# RUN: python %s > %t
+# RUN: cat %t | xargs not %clang                     2>&1 | FileCheck %s --check-prefix=PHYSICAL
+# RUN: cat %t | xargs not %clang -ivfsmode=physical  2>&1 | FileCheck %s --check-prefix=PHYSICAL
+# RUN: cat %t | xargs     %clang -ivfsmode=real
+
+# PHYSICAL: error: no such file or directory:
+
+import os
+import subprocess
+
+current = os.path.realpath(os.curdir)
+name_max = int(subprocess.check_output("getconf NAME_MAX %s" % current, shell=True))
+path_max = int(subprocess.check_output("getconf PATH_MAX %s" % current, shell=True))
+assert name_max > 0
+assert path_max > 0
+
+filename = 'o.c'
+code = 'int main() { return 0; }'
+
+relative = ''
+absolute = current
+
+# Create directories which almost, but not quite, exhaust PATH_MAX.
+target_len = path_max - 1
+while len(absolute) < target_len:
+    new_name_len = target_len - len(absolute) - 1
+    new_name_len = new_name_len if new_name_len < name_max else name_max
+    absolute = os.path.join(absolute, 'x' * new_name_len)
+    relative = os.path.join(relative, 'x' * new_name_len)
+    os.mkdir(relative)
+    assert os.path.exists(absolute)
+    assert os.path.exists(relative)
+
+# Create a file whose relative path doesn't exceed PATH_MAX, but whose absolute
+# path does.
+file_relative_path = os.path.join(relative, filename)
+with open(file_relative_path, 'w') as f:
+    f.write(code)
+
+# At this point only the relative path should work. The absolute one should
+# exceed PATH_MAX.
+assert os.path.exists(file_relative_path)
+
+# Print out the relative path so lit can pass to clang.
+print file_relative_path