[clang-cl] Implement /external:I, /external:env, and EXTERNAL_INCLUDE support (PR36003)

[hans]

Summary

This patch does three things:

• Map the /external:I flag to -isystem

• Add support for the /external:env:<var> flag which reads system include paths for the <var> environment variable

• Pick up system include dirs EXTERNAL_INCLUDE in addition to the old INCLUDE environment variable.

[thakis]

Nice, that's much less convoluted than I had feared :)

clang/lib/Driver/ToolChains/MSVC.cpp

1255

/external:env should happen after winsysroot I think. sysroots try to make builds hermetic and env vars defeat that.

I.e. this flag is more like the env var reads below than imsvc above – it reads the env mostly, instead of being a flag mostly.

1278

Maybe this should grow a FIXME to make regular INCLUDE not a system include at some point? Kind of sounds like this is the direction msvc is moving in with the existence of EXTERNAL_INCLUDE at least.

clang/test/Driver/cl-include.c

10

Should there be tests for the interaction with /X, /winsysroot:?

[hans]

More tests, add help text for command-line options.

[hans]

clang/lib/Driver/ToolChains/MSVC.cpp

1255

Hmm. I guess it depends on how people will use this flag, which isn't entirely clear.

The way I think about it, users might use this to point to third-party libraries besides the system headers. For that reason it's different from the env var reads below, which are about finding the system headers.

In particular, I don't think /nostdlibinc (/X) should disable these flags, which is why I put it before that check.

I don't think people would combine this with /winsysroot, but if they want to, I'm not sure we should prevent it.

clang/test/Driver/cl-include.c

10

Yes, adding that.

[thakis]

That's a good argument. lgtm.

clang/lib/Driver/ToolChains/MSVC.cpp

1255

I don't know what people in general want, but _I_ definitely want a mode that makes the compiler not read any env vars :) But fair enough, this flag explicitly opts in to reading env vars, so if I don't want that I can just not pass this flag.

1273

Why not keep the definition of include_var in the if condition like it was on the rhs? (And do it for ext_include_var too)? They're only used in the respective if body, right? i.e. like so:

if (llvm::Optional<std::string> include_var =
        llvm::sys::Process::GetEnv("INCLUDE")) {
  StringRef(*include_var)
          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
}
if (llvm::Optional<std::string> ext_include_var =
        llvm::sys::Process::GetEnv("EXTERNAL_INCLUDE")) {
  StringRef(*ext_include_var)
          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
}

(Might even do for (auto s : {"INCLUDE", "EXTERNAL_INCLUDE"}) ... but that makes the FIXME harder to do later, so maybe don't do that part :) )

[hans]

clang/lib/Driver/ToolChains/MSVC.cpp

1273

It's because split() creates StringRefs into include_var and ext_include_var, so they need to stay in scope until we're done with the Dirs object.

But that's pretty subtle, and the code doesn't look very nice. We're doing the same "get env var and split" dance three times now, so I'll move it to a helper lambda.

clang/include/clang/Driver/Options.td

def _SLASH_diagnostics_caret : CLFlag<"diagnostics:caret">,
   HelpText<"Enable caret and column diagnostics (default)">;
 def _SLASH_diagnostics_column : CLFlag<"diagnostics:column">,
   HelpText<"Disable caret diagnostics but keep column info">;
 def _SLASH_diagnostics_classic : CLFlag<"diagnostics:classic">,
   HelpText<"Disable column and caret diagnostics">;
 def _SLASH_D : CLJoinedOrSeparate<"D">, HelpText<"Define macro">,
   MetaVarName<"<macro[=value]>">, Alias<D>;
 def _SLASH_E : CLFlag<"E">, HelpText<"Preprocess to stdout">, Alias<E>;
+def _SLASH_external_COLON_I : CLJoinedOrSeparate<"external:I">, Alias<isystem>,
+  HelpText<"Add directory to include search path with warnings suppressed">,
+  MetaVarName<"<dir>">;
 def _SLASH_fp_except : CLFlag<"fp:except">, HelpText<"">, Alias<ftrapping_math>;
 def _SLASH_fp_except_ : CLFlag<"fp:except-">,
   HelpText<"">, Alias<fno_trapping_math>;
 def _SLASH_fp_fast : CLFlag<"fp:fast">, HelpText<"">, Alias<ffast_math>;
 def _SLASH_fp_precise : CLFlag<"fp:precise">,
   HelpText<"">, Alias<fno_fast_math>;
 def _SLASH_fp_strict : CLFlag<"fp:strict">, HelpText<"">, Alias<fno_fast_math>;
 def _SLASH_fsanitize_EQ_address : CLFlag<"fsanitize=address">,

 
 def _SLASH_M_Group : OptionGroup<"</M group>">, Group<cl_compile_Group>;
 def _SLASH_volatile_Group : OptionGroup<"</volatile group>">,
   Group<cl_compile_Group>;
 
 def _SLASH_EH : CLJoined<"EH">, HelpText<"Set exception handling model">;
 def _SLASH_EP : CLFlag<"EP">,
   HelpText<"Disable linemarker output and preprocess to stdout">;
+def _SLASH_external_env : CLJoined<"external:env:">,
+  HelpText<"Add dirs in env var <var> to include search path with warnings suppressed">,
+  MetaVarName<"<var>">;
 def _SLASH_FA : CLFlag<"FA">,
   HelpText<"Output assembly code file during compilation">;
 def _SLASH_Fa : CLJoined<"Fa">,
   HelpText<"Set assembly output file name (with /FA)">,
   MetaVarName<"<file or dir/>">;
 def _SLASH_FI : CLJoinedOrSeparate<"FI">,
   HelpText<"Include file before parsing">, Alias<include_>;
 def _SLASH_Fe : CLJoined<"Fe">,

 def _SLASH_Bt : CLFlag<"Bt">;
 def _SLASH_Bt_plus : CLFlag<"Bt+">;
 def _SLASH_clr : CLJoined<"clr">;
 def _SLASH_d2 : CLJoined<"d2">;
 def _SLASH_doc : CLJoined<"doc">;
 def _SLASH_experimental : CLJoined<"experimental:">;
 def _SLASH_exportHeader : CLFlag<"exportHeader">;
 def _SLASH_external : CLJoined<"external:">;
-def _SLASH_external_COLON_I : CLJoinedOrSeparate<"external:I">;
 def _SLASH_FA_joined : CLJoined<"FA">;
 def _SLASH_favor : CLJoined<"favor">;
 def _SLASH_fsanitize_address_use_after_return : CLJoined<"fsanitize-address-use-after-return">;
 def _SLASH_fno_sanitize_address_vcasan_lib : CLJoined<"fno-sanitize-address-vcasan-lib">;
 def _SLASH_F : CLJoinedOrSeparate<"F">;
 def _SLASH_Fm : CLJoined<"Fm">;
 def _SLASH_Fr : CLJoined<"Fr">;
 def _SLASH_FR : CLJoined<"FR">;

clang/lib/Driver/ToolChains/MSVC.cpp

if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
     AddSystemIncludeWithSubfolder(DriverArgs, CC1Args, getDriver().ResourceDir,
                                   "include");
   }
 
   // Add %INCLUDE%-like directories from the -imsvc flag.
   for (const auto &Path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
     addSystemInclude(DriverArgs, CC1Args, Path);
 
+  // Add %INCLUDE%-like dirs via the /external:env: flag.
+  for (const auto &Var :
+       DriverArgs.getAllArgValues(options::OPT__SLASH_external_env)) {
+    if (auto Val = llvm::sys::Process::GetEnv(Var)) {
+      SmallVector<StringRef, 8> Dirs;
+      StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+      for (StringRef Dir : Dirs)
+        addSystemInclude(DriverArgs, CC1Args, Dir);
+    }
+  }
+
   if (DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
-  // Honor %INCLUDE%. It should know essential search paths with vcvarsall.bat.
-  // Skip if the user expressly set a vctoolsdir
+  // Honor %INCLUDE% and %EXTERNAL_INCLUDE%. It should have essential search
+  // paths set by vcvarsall.bat. Skip if the user expressly set a vctoolsdir.
   if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
                              options::OPT__SLASH_winsysroot)) {
-    if (llvm::Optional<std::string> cl_include_dir =
-            llvm::sys::Process::GetEnv("INCLUDE")) {
-      SmallVector<StringRef, 8> Dirs;
-      StringRef(*cl_include_dir)
+    llvm::Optional<std::string> include_var =
+        llvm::sys::Process::GetEnv("INCLUDE");
+    // FIXME: MSVC seems to be moving from INCLUDE to EXTERNAL_INCLUDE for
+    // system includes. Perhaps INCLUDE paths should not be considered systems
+    // paths at some point in the future.
+    llvm::Optional<std::string> ext_include_var =
+        llvm::sys::Process::GetEnv("EXTERNAL_INCLUDE");
+
+    if (include_var) {
+      StringRef(*include_var)
           .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
-      for (StringRef Dir : Dirs)
-        addSystemInclude(DriverArgs, CC1Args, Dir);
-      if (!Dirs.empty())
-        return;
+    }
+    if (ext_include_var) {
+      StringRef(*ext_include_var)
+          .split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+    }
+
+    if (!Dirs.empty()) {
+      addSystemIncludes(DriverArgs, CC1Args, Dirs);
     }
   }
 
   // When built with access to the proper Windows APIs, try to actually find
   // the correct include paths first.
   if (!VCToolChainPath.empty()) {
     addSystemInclude(DriverArgs, CC1Args,
                      getSubDirectoryPath(SubDirectoryType::Include));

clang/test/Driver/cl-include.c

 // Note: %s must be preceded by --, otherwise it may be interpreted as a
 // command-line option, e.g. on Mac where %s is commonly under /Users.
 
 // RUN: %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=BUILTIN
 // BUILTIN: "-internal-isystem" "{{.*lib.*clang.*include}}"
 
 // RUN: %clang_cl -nobuiltininc -### -- %s 2>&1 | FileCheck %s --check-prefix=NOBUILTIN
 // NOBUILTIN-NOT: "-internal-isystem" "{{.*lib.*clang.*include}}"
 
-// RUN: env INCLUDE=/my/system/inc %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=STDINC
+// RUN: env INCLUDE=/my/system/inc env EXTERNAL_INCLUDE=/my/system/inc2 %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=STDINC
 // STDINC: "-internal-isystem" "/my/system/inc"
+// STDINC: "-internal-isystem" "/my/system/inc2"
 
 // -nostdinc suppresses all of %INCLUDE%, clang resource dirs, and -imsvc dirs.
-// RUN: env INCLUDE=/my/system/inc %clang_cl -nostdinc -imsvc /my/other/inc -### -- %s 2>&1 | FileCheck %s --check-prefix=NOSTDINC
+// RUN: env INCLUDE=/my/system/inc env EXTERNAL_INCLUDE=/my/system/inc2 %clang_cl -nostdinc -imsvc /my/other/inc -### -- %s 2>&1 | FileCheck %s --check-prefix=NOSTDINC
 // NOSTDINC: argument unused{{.*}}-imsvc
 // NOSTDINC-NOT: "-internal-isystem" "/my/system/inc"
+// NOSTDINC-NOT: "-internal-isystem" "/my/system/inc2"
 // NOSTDINC-NOT: "-internal-isystem" "{{.*lib.*clang.*include}}"
 // NOSTDINC-NOT: "-internal-isystem" "/my/other/inc"
 
-// /X suppresses %INCLUDE% but not clang resource dirs or -imsvc dirs.
-// RUN: env INCLUDE=/my/system/inc %clang_cl /X -imsvc /my/other/inc -### -- %s 2>&1 | FileCheck %s --check-prefix=SLASHX
+// /X suppresses %INCLUDE% and %EXTERNAL_INCLUDE% but not clang resource dirs, -imsvc dirs, or /external: flags.
+// RUN: env INCLUDE=/my/system/inc env EXTERNAL_INCLUDE=/my/system/inc2 env FOO=/my/other/inc2 %clang_cl /X -imsvc /my/other/inc /external:env:FOO -### -- %s 2>&1 | FileCheck %s --check-prefix=SLASHX
 // SLASHX-NOT: "argument unused{{.*}}-imsvc"
 // SLASHX-NOT: "-internal-isystem" "/my/system/inc"
+// SLASHX-NOT: "-internal-isystem" "/my/system/inc2"
 // SLASHX: "-internal-isystem" "{{.*lib.*clang.*include}}"
 // SLASHX: "-internal-isystem" "/my/other/inc"
+// SLASHX: "-internal-isystem" "/my/other/inc2"
+
+// /winsysroot suppresses %EXTERNAL_INCLUDE% but not -imsvc dirs or /external: flags.
+// RUN: env env EXTERNAL_INCLUDE=/my/system/inc env FOO=/my/other/inc2 %clang_cl /winsysroot /foo -imsvc /my/other/inc /external:env:FOO -### -- %s 2>&1 | FileCheck %s --check-prefix=SYSROOT
+// SYSROOT-NOT: "argument unused{{.*}}-imsvc"
+// SYSROOT-NOT: "argument unused{{.*}}/external:"
+// SYSROOT-NOT: "/my/system/inc"
+// SYSROOT: "-internal-isystem" "/my/other/inc"
+// SYSROOT: "-internal-isystem" "/my/other/inc2"
+// SYSROOT: "-internal-isystem" "/foo{{.*}}"
+
+// RUN: env "FOO=/dir1;/dir2" env "BAR=/dir3" %clang_cl /external:env:FOO /external:env:BAR -### -- %s 2>&1 | FileCheck %s --check-prefix=EXTERNAL_ENV
+// EXTERNAL_ENV: "-internal-isystem" "/dir1"
+// EXTERNAL_ENV: "-internal-isystem" "/dir2"
+// EXTERNAL_ENV: "-internal-isystem" "/dir3"

clang/test/Driver/cl-options.c

 // E: "-E"
 // E: "-o" "-"
 
 // RUN: %clang_cl /EP -### -- %s 2>&1 | FileCheck -check-prefix=EP %s
 // EP: "-E"
 // EP: "-P"
 // EP: "-o" "-"
 
+// RUN: %clang_cl /external:Ipath  -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_I %s
+// RUN: %clang_cl /external:I path -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_I %s
+// EXTERNAL_I: "-isystem" "path"
+
 // RUN: %clang_cl /fp:fast /fp:except -### -- %s 2>&1 | FileCheck -check-prefix=fpexcept %s
 // fpexcept-NOT: -menable-unsafe-fp-math
 
 // RUN: %clang_cl /fp:fast /fp:except /fp:except- -### -- %s 2>&1 | FileCheck -check-prefix=fpexcept_ %s
 // fpexcept_: -menable-unsafe-fp-math
 
 // RUN: %clang_cl /fp:precise /fp:fast -### -- %s 2>&1 | FileCheck -check-prefix=fpfast %s
 // fpfast: -menable-unsafe-fp-math

 // RUN:     /clr:pure \
 // RUN:     /d2FH4 \
 // RUN:     /docname \
 // RUN:     /experimental:external \
 // RUN:     /experimental:module \
 // RUN:     /experimental:preprocessor \
 // RUN:     /exportHeader /headerName:foo \
 // RUN:     /external:anglebrackets \
-// RUN:     /external:Ipath \
-// RUN:     /external:I path \
 // RUN:     /external:env:var \
 // RUN:     /external:W0 \
 // RUN:     /external:W1 \
 // RUN:     /external:W2 \
 // RUN:     /external:W3 \
 // RUN:     /external:W4 \
 // RUN:     /external:templates- \
 // RUN:     /headerUnit foo.h=foo.ifc /headerUnit:quote foo.h=foo.ifc /headerUnit:angle foo.h=foo.ifc \