[Clang] Match MSVC handling of duplicate header search paths in Microsoft compatibility modes.

Clang has historically matched GCC's behavior for header search path order
and pruning of duplicate paths. That traditional behavior is to order user
search paths before system search paths, to ignore user search paths that
duplicate a (later) system search path, and to ignore search paths that
duplicate an earlier search path of the same user/system kind. This differs
from MSVC and can result in inconsistent header file resolution for `#include`
directives.

MSVC orders header search paths as follows:
1) Paths specified by the `/I` and `/external:I` options are processed in
   the order that they appear. Paths specified by `/I` that duplicate a path
   specified by `/external:I` are ignored regardless of the order of the
   options. Paths specified by `/I` that duplicate a path from a prior `/I`
   option are ignored. Paths specified by `/external:I` that duplicate a
   path from a later `/external:I` option are ignored.
2) Paths specified by `/external:env` are processed in the order that they
   appear. Paths that duplicate a path from a `/I` or `/external:I` option
   are ignored regardless of the order of the options. Paths that duplicate a
   path from a prior `/external:env` option or an earlier path from the same
   `/external:env` option are ignored.
3) Paths specified by the `INCLUDE` environment variable are processed in
   the order they appear. Paths that duplicate a path from a `/I`,
   `/external:I`, or `/external:env` option are ignored. Paths that
   duplicate an earlier path in the `INCLUDE` environment variable are
   ignored.
4) Paths specified by the `EXTERNAL_INCLUDE` environment variable are
   processed in the order they appear. Paths that duplicate a path from a
   `/I`, `/external:I`, or `/external:env` option are ignored. Paths that
   duplicate a path from the `INCLUDE` environment variable are ignored.
   Paths that duplicate an earlier path in the `EXTERNAL_INCLUDE
   environment variable are ignored.

Prior to this change, Clang handled the `/external:I` and `/external:env`
options and the paths present in the `INCLUDE` and `EXTERNAL_INCLUDE`
environment variables as though they were specified with the `-isystem` option.
The GCC behavior described above then lead to a command line such as
`/external:I dir1 /Idir2` having a header search order of `dir2` followed by
`dir1`; contrary to MSVC behavior.

This change adds support for the MSVC external path concept for both the `clang`
and `clang-cl` drivers with the following option syntax. These options match the
MSVC behavior described above for both drivers.
                 clang                    clang-cl
                 --------------------     -------------------
                 -iexternal <dir>         /external:I <dir>
                 -iexternal-env=<ENV>     /external:env:<ENV>
Paths specified by these options are still treated as system paths. That is,
whether warnings are issued in header files found via these paths remains
subject to use of the `-Wsystem-headers` and `-Wno-system-headers` options.
In the future, it would make sense to add a separate option that matches the
MSVC `/external:Wn` option to control such warnings.

The MSVC behavior described above implies that (system) paths present in the
`INCLUDE` and `EXTERNAL_INCLUDE` environment variables do not suppress matching
user paths specified via `/I`. This contrasts with GCC's behavior of suppressing
user paths that match a system path regardless of how each is specified. Since
the `clang-cl` driver maps paths from the `INCLUDE` and `EXTERNAL_INCLUDE`
environment variable to `-internal-isystem`, matching MSVC behavior requires
suppressing that aspect of the GCC behavior. With this change, system paths
will no longer suppress user paths when the `-fms-compatibility` option is
explicitly or implicitly enabled. This will affect header search path ordering
for options like `-isystem` when duplicate user paths are present. Should
motivation arise for preserving such suppression of user paths when compiling
with `-fms-compatibility` enabled, it would make sense to introduce a new
option for the `clang-cl` driver to map paths in these environment variabless
to that would match MSVC behavior without impacting other system path options.

Author: tahonermann

clang/docs/ReleaseNotes.rst

@@ -681,6 +681,44 @@ Windows Support
   When `-fms-compatibility-version=18.00` or prior is set on the command line this Microsoft extension is still
   allowed as VS2013 and prior allow it.
 
+- Clang now matches MSVC behavior regarding the handling of duplicate header
+  search paths when running in Microsoft compatibility mode. Historically,
+  Clang has mimicked gcc behavior in which user search paths are ordered before
+  system search paths, user search paths that duplicate a (later) system search
+  path are ignored, and search paths that duplicate an earlier search path of
+  the same user/system kind are ignored. This ordering is not compatible with
+  the ordering that MSVC uses when paths are duplicated across ``/I`` options
+  and the ``INCLUDE`` environment variable.
+
+  The order that MSVC uses and that Clang now replicates when the
+  ``-fms-compatibility`` option is enabled follows.
+
+  - Paths specified by the ``/I`` and ``/external:I`` options are processed in
+    the order that they appear. Paths specified by ``/I`` that duplicate a path
+    specified by ``/external:I`` are ignored regardless of the order of the
+    options. Paths specified by ``/I`` that duplicate a path from a prior ``/I``
+    option are ignored. Paths specified by ``/external:I`` that duplicate a
+    path from a later ``/external:I`` option are ignored.
+
+  - Paths specified by ``/external:env`` are processed in the order that they
+    appear. Paths that duplicate a path from a ``/I`` or ``/external:I`` option
+    are ignored regardless of the order of the options. Paths that duplicate a
+    path from a prior ``/external:env`` option or an earlier path from the same
+    ``/external:env`` option are ignored.
+
+  - Paths specified by the ``INCLUDE`` environment variable are processed in
+    the order they appear. Paths that duplicate a path from a ``/I``,
+    ``/external:I``, or ``/external:env`` option are ignored. Paths that
+    duplicate an earlier path in the ``INCLUDE`` environment variable are
+    ignored.
+
+  - Paths specified by the ``EXTERNAL_INCLUDE`` environment variable are
+    processed in the order they appear. Paths that duplicate a path from a
+    ``/I``, ``/external:I``, or ``/external:env`` option are ignored. Paths that
+    duplicate a path from the ``INCLUDE`` environment variable are ignored.
+    Paths that duplicate an earlier path in the ``EXTERNAL_INCLUDE``
+    environment variable are ignored.
+
 LoongArch Support
 ^^^^^^^^^^^^^^^^^
 

clang/include/clang/Driver/Options.td

@@ -4579,6 +4579,15 @@ def iapinotes_modules : JoinedOrSeparate<["-"], "iapinotes-modules">, Group<clan
 def idirafter : JoinedOrSeparate<["-"], "idirafter">, Group<clang_i_Group>,
   Visibility<[ClangOption, CC1Option]>,
   HelpText<"Add directory to AFTER include search path">;
+def iexternal : Separate<["-"], "iexternal">, Group<clang_i_Group>,
+  Visibility<[ClangOption, CC1Option]>,
+  HelpText<"Add directory to include search path with warnings suppressed">, MetaVarName<"<dir>">;
+def iexternal_after : Separate<["-"], "iexternal-after">, Group<clang_i_Group>,
+  Visibility<[ClangOption, CC1Option]>,
+  HelpText<"Add directory to include search path with warnings suppressed">, MetaVarName<"<dir>">;
+def iexternal_env_EQ : Joined<["-"], "iexternal-env=">, Group<clang_i_Group>,
+  Visibility<[ClangOption]>,
+  HelpText<"Add dirs in env var <var> to include search path with warnings suppressed">, MetaVarName<"<var>">;
 def iframework : JoinedOrSeparate<["-"], "iframework">, Group<clang_i_Group>,
   Visibility<[ClangOption, CC1Option]>,
   HelpText<"Add directory to SYSTEM framework search path">;
@@ -8460,9 +8469,12 @@ def _SLASH_diagnostics_classic : CLFlag<"diagnostics:classic">,
 def _SLASH_D : CLJoinedOrSeparate<"D", [CLOption, DXCOption]>,
   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>,
+def _SLASH_external_COLON_I : CLJoinedOrSeparate<"external:I">, Alias<iexternal>,
   HelpText<"Add directory to include search path with warnings suppressed">,
   MetaVarName<"<dir>">;
+def _SLASH_external_env : CLJoined<"external:env:">, Alias<iexternal_env_EQ>,
+  HelpText<"Add dirs in env var <var> to include search path with warnings suppressed">,
+  MetaVarName<"<var>">;
 def _SLASH_fp_contract : CLFlag<"fp:contract">, HelpText<"">, Alias<ffp_contract>, AliasArgs<["on"]>;
 def _SLASH_fp_except : CLFlag<"fp:except">, HelpText<"">, Alias<ffp_exception_behavior_EQ>, AliasArgs<["strict"]>;
 def _SLASH_fp_except_ : CLFlag<"fp:except-">, HelpText<"">, Alias<ffp_exception_behavior_EQ>, AliasArgs<["ignore"]>;
@@ -8692,9 +8704,6 @@ def _SLASH_volatile_Group : OptionGroup<"</volatile 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 : CLJoined<"FA">,
   HelpText<"Output assembly code file during compilation">;
 def _SLASH_Fa : CLJoined<"Fa">,

clang/include/clang/Driver/ToolChain.h

@@ -224,6 +224,7 @@ class ToolChain {
   /// \return The subdirectory path if it exists.
   std::optional<std::string> getTargetSubDirPath(StringRef BaseDir) const;
 
+public:
   /// \name Utilities for implementing subclasses.
   ///@{
   static void addSystemInclude(const llvm::opt::ArgList &DriverArgs,
@@ -239,12 +240,20 @@ class ToolChain {
   static void addSystemIncludes(const llvm::opt::ArgList &DriverArgs,
                                 llvm::opt::ArgStringList &CC1Args,
                                 ArrayRef<StringRef> Paths);
+  static bool addSystemIncludesFromEnv(const llvm::opt::ArgList &DriverArgs,
+                                       llvm::opt::ArgStringList &CC1Args,
+                                       StringRef Var);
+  static void addExternalAfterIncludes(const llvm::opt::ArgList &DriverArgs,
+                                       llvm::opt::ArgStringList &CC1Args,
+                                       ArrayRef<StringRef> Paths);
+  static bool addExternalIncludesFromEnv(const llvm::opt::ArgList &DriverArgs,
+                                         llvm::opt::ArgStringList &CC1Args,
+                                         StringRef Var);
 
   static std::string concat(StringRef Path, const Twine &A, const Twine &B = "",
                             const Twine &C = "", const Twine &D = "");
   ///@}
 
-public:
   virtual ~ToolChain();
 
   // Accessors

clang/include/clang/Lex/HeaderSearchOptions.h

@@ -38,6 +38,16 @@ enum IncludeDirGroup {
   /// Like Angled, but marks header maps used when building frameworks.
   IndexHeaderMap,
 
+  /// Like Angled, but marks system directories while retaining relative order
+  /// with user directories. This group is intended to match the semantics of
+  /// the MSVC /external:I option.
+  External,
+
+  /// Like External, but searched after other external directories but before
+  /// system directories. This group is intended to match the semantics of the
+  /// MSVC /external:env option.
+  ExternalAfter,
+
   /// Like Angled, but marks system directories.
   System,
 

clang/lib/Driver/Driver.cpp

@@ -1296,7 +1296,9 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
     if (VFS->setCurrentWorkingDirectory(WD->getValue()))
       Diag(diag::err_drv_unable_to_set_working_directory) << WD->getValue();
 
-  // Check for missing include directories.
+  // Check for missing include directories. Diagnostics should not be issued
+  // for directories specified with -iexternal, -iexternal-env=, or
+  // -iexternal-after since those options may be used to specify partial paths.
   if (!Diags.isIgnored(diag::warn_missing_include_dirs, SourceLocation())) {
     for (auto IncludeDir : Args.getAllArgValues(options::OPT_I_Group)) {
       if (!VFS->exists(IncludeDir))

clang/lib/Driver/ToolChain.cpp

@@ -1267,6 +1267,51 @@ void ToolChain::addExternCSystemIncludeIfExists(const ArgList &DriverArgs,
   }
 }
 
+/// Utility function to add a list of ';' delimited directories specified in
+/// an environment variable to the system include path list for CC1. Returns
+/// true if the variable is set and not empty.
+/*static*/ bool ToolChain::addSystemIncludesFromEnv(const ArgList &DriverArgs,
+                                                    ArgStringList &CC1Args,
+                                                    StringRef Var) {
+  if (auto Val = llvm::sys::Process::GetEnv(Var)) {
+    SmallVector<StringRef, 8> Dirs;
+    StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+    if (!Dirs.empty()) {
+      addSystemIncludes(DriverArgs, CC1Args, Dirs);
+      return true;
+    }
+  }
+  return false;
+}
+
+/// Utility function to add a list of directories to the end of the external
+/// include path list for CC1.
+/*static*/ void ToolChain::addExternalAfterIncludes(const ArgList &DriverArgs,
+                                                    ArgStringList &CC1Args,
+                                                    ArrayRef<StringRef> Paths) {
+  for (const auto &Path : Paths) {
+    CC1Args.push_back("-iexternal-after");
+    CC1Args.push_back(DriverArgs.MakeArgString(Path));
+  }
+}
+
+/// Utility function to add a list of ';' delimited directories specified in
+/// an environment variable to the external include path list for CC1. Returns
+/// true if the variable is set and not empty.
+/*static*/ bool ToolChain::addExternalIncludesFromEnv(const ArgList &DriverArgs,
+                                                      ArgStringList &CC1Args,
+                                                      StringRef Var) {
+  if (auto Val = llvm::sys::Process::GetEnv(Var)) {
+    SmallVector<StringRef, 8> Dirs;
+    StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+    if (!Dirs.empty()) {
+      addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
+      return true;
+    }
+  }
+  return false;
+}
+
 /*static*/ std::string ToolChain::concat(StringRef Path, const Twine &A,
                                          const Twine &B, const Twine &C,
                                          const Twine &D) {

clang/lib/Driver/ToolChains/Clang.cpp

@@ -1196,6 +1196,13 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
     } else if (A->getOption().matches(options::OPT_ibuiltininc)) {
       // This is used only by the driver. No need to pass to cc1.
       continue;
+    } else if (A->getOption().matches(options::OPT_iexternal)) {
+      // This option has to retain relative order with other -I options.
+      continue;
+    } else if (A->getOption().matches(options::OPT_iexternal_env_EQ)) {
+      A->claim();
+      ToolChain::addExternalIncludesFromEnv(Args, CmdArgs, A->getValue());
+      continue;
     }
 
     // Not translated, render as usual.
@@ -1206,7 +1213,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   Args.addAllArgs(CmdArgs,
                   {options::OPT_D, options::OPT_U, options::OPT_I_Group,
                    options::OPT_F, options::OPT_index_header_map,
-                   options::OPT_embed_dir_EQ});
+                   options::OPT_iexternal, options::OPT_embed_dir_EQ});
 
   // Add -Wp, and -Xpreprocessor if using the preprocessor.
 
@@ -8663,7 +8670,7 @@ void ClangAs::ConstructJob(Compilation &C, const JobAction &JA,
   (void)Args.hasArg(options::OPT_force__cpusubtype__ALL);
 
   // Pass along any -I options so we get proper .include search paths.
-  Args.AddAllArgs(CmdArgs, options::OPT_I_Group);
+  Args.addAllArgs(CmdArgs, {options::OPT_I_Group, options::OPT_iexternal});
 
   // Pass along any --embed-dir or similar options so we get proper embed paths.
   Args.AddAllArgs(CmdArgs, options::OPT_embed_dir_EQ);

clang/lib/Driver/ToolChains/MSVC.cpp

@@ -648,24 +648,6 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   for (const auto &Path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
     addSystemInclude(DriverArgs, CC1Args, Path);
 
-  auto AddSystemIncludesFromEnv = [&](StringRef Var) -> bool {
-    if (auto Val = llvm::sys::Process::GetEnv(Var)) {
-      SmallVector<StringRef, 8> Dirs;
-      StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
-      if (!Dirs.empty()) {
-        addSystemIncludes(DriverArgs, CC1Args, Dirs);
-        return true;
-      }
-    }
-    return false;
-  };
-
-  // Add %INCLUDE%-like dirs via /external:env: flags.
-  for (const auto &Var :
-       DriverArgs.getAllArgValues(options::OPT__SLASH_external_env)) {
-    AddSystemIncludesFromEnv(Var);
-  }
-
   // Add DIA SDK include if requested.
   if (const Arg *A = DriverArgs.getLastArg(options::OPT__SLASH_diasdkdir,
                                            options::OPT__SLASH_winsysroot)) {
@@ -682,12 +664,14 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   if (DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
-  // Honor %INCLUDE% and %EXTERNAL_INCLUDE%. It should have essential search
-  // paths set by vcvarsall.bat. Skip if the user expressly set a vctoolsdir.
+  // Add paths from the INCLUDE and EXTERNAL_INCLUDE environment variables if
+  // neither a vctoolsdir or winsysroot directory has been explicitly specified.
+  // If any paths are present in these environment variables, then skip adding
+  // additional system directories.
   if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
                              options::OPT__SLASH_winsysroot)) {
-    bool Found = AddSystemIncludesFromEnv("INCLUDE");
-    Found |= AddSystemIncludesFromEnv("EXTERNAL_INCLUDE");
+    bool Found = addSystemIncludesFromEnv(DriverArgs, CC1Args, "INCLUDE");
+    Found |= addSystemIncludesFromEnv(DriverArgs, CC1Args, "EXTERNAL_INCLUDE");
     if (Found)
       return;
   }

clang/lib/Frontend/CompilerInvocation.cpp

@@ -3190,8 +3190,11 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
   auto It = Opts.UserEntries.begin();
   auto End = Opts.UserEntries.end();
 
-  // Add -I..., -F..., and -index-header-map options in order.
-  for (; It < End && Matches(*It, {frontend::IndexHeaderMap, frontend::Angled},
+  // Add the -I..., -F..., -index-header-map, and -iexternal options
+  // in order.
+  for (; It < End && Matches(*It,
+                             {frontend::IndexHeaderMap, frontend::Angled,
+                              frontend::External},
                              std::nullopt, true);
        ++It) {
     OptSpecifier Opt = [It, Matches]() {
@@ -3203,13 +3206,20 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
         return OPT_F;
       if (Matches(*It, frontend::Angled, false, true))
         return OPT_I;
+      if (Matches(*It, frontend::External, false, true))
+        return OPT_iexternal;
       llvm_unreachable("Unexpected HeaderSearchOptions::Entry.");
     }();
 
     if (It->Group == frontend::IndexHeaderMap)
       GenerateArg(Consumer, OPT_index_header_map);
     GenerateArg(Consumer, Opt, It->Path);
-  };
+  }
+
+  // Add the paths for the -iexternal-env= and -iexternal-after options in
+  // order.
+  for (; It < End && Matches(*It, {frontend::ExternalAfter}, false, true); ++It)
+    GenerateArg(Consumer, OPT_iexternal_after, It->Path);
 
   // Note: some paths that came from "[-iprefix=xx] -iwithprefixbefore=yy" may
   // have already been generated as "-I[xx]yy". If that's the case, their
@@ -3319,8 +3329,6 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
         llvm::CachedHashString(MacroDef.split('=').first));
   }
 
-  // Add -I..., -F..., and -index-header-map options in order.
-  bool IsIndexHeaderMap = false;
   bool IsSysrootSpecified =
       Args.hasArg(OPT__sysroot_EQ) || Args.hasArg(OPT_isysroot);
 
@@ -3339,22 +3347,32 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
     return A->getValue();
   };
 
-  for (const auto *A : Args.filtered(OPT_I, OPT_F, OPT_index_header_map)) {
+  // Add the -I..., -F..., -index-header-map, and -iexternal options in order.
+  bool IsIndexHeaderMap = false;
+  for (const auto *A :
+       Args.filtered(OPT_I, OPT_F, OPT_index_header_map, OPT_iexternal)) {
+    frontend::IncludeDirGroup Group =
+        IsIndexHeaderMap ? frontend::IndexHeaderMap : frontend::Angled;
+
     if (A->getOption().matches(OPT_index_header_map)) {
       // -index-header-map applies to the next -I or -F.
       IsIndexHeaderMap = true;
       continue;
     }
-
-    frontend::IncludeDirGroup Group =
-        IsIndexHeaderMap ? frontend::IndexHeaderMap : frontend::Angled;
+    if (A->getOption().matches(OPT_iexternal))
+      Group = frontend::External;
 
     bool IsFramework = A->getOption().matches(OPT_F);
     Opts.AddPath(PrefixHeaderPath(A, IsFramework), Group, IsFramework,
                  /*IgnoreSysroot*/ true);
     IsIndexHeaderMap = false;
   }
 
+  // Add the -iexternal-env= and -iexternal-after options in order.
+  for (const auto *A : Args.filtered(OPT_iexternal_after))
+    Opts.AddPath(A->getValue(), frontend::ExternalAfter,
+                 /*IsFramework=*/false, /*IgnoreSysRoot=*/true);
+
   // Add -iprefix/-iwithprefix/-iwithprefixbefore options.
   StringRef Prefix = ""; // FIXME: This isn't the correct default prefix.
   for (const auto *A :