[clang] [llvm] [Clang] Match MSVC handling of duplicate header search paths in Microsoft compatibility modes. (PR #105738)

Tom Honermann via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 25 11:41:08 PDT 2024


https://github.com/tahonermann updated https://github.com/llvm/llvm-project/pull/105738

>From b775b7288035d83281434e27341b98a76623802f Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Thu, 22 Aug 2024 09:44:56 -0700
Subject: [PATCH 1/9] [Clang] Match MSVC handling of duplicate header search
 paths in Microsoft compatibility modes.

Clang has historically mimicked gcc behavior for header file searching 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.

MSVC behavior differs in that user search paths are ordered before system
search paths (like gcc), and search paths that duplicate an earlier search
path are ignored regardless of user/system kind (similar to gcc, but without
the preference for system search paths over duplicate user search paths).

The gcc and MSVC differences are observable for driver invocations that pass,
e.g., `-Idir1 -isystem dir2 -isystem dir1`. The gcc behavior will result in
`dir2` being searched before `dir1` while the MSVC behavior will result in
`dir1` being searched before `dir2`.

This patch modifies Clang to match the MSVC behavior for handling of duplicate
header search paths when running in Microsoft compatibility mode (e.g., when
invoked with the `-fms-compatibility` option explicitly or implicitly enabled).
---
 clang/docs/ReleaseNotes.rst                   | 14 +++
 clang/lib/Lex/InitHeaderSearch.cpp            | 21 ++--
 clang/test/Driver/header-search-duplicates.c  | 76 +++++++++++++++
 .../microsoft-header-search-duplicates.c      | 97 +++++++++++++++++++
 4 files changed, 199 insertions(+), 9 deletions(-)
 create mode 100644 clang/test/Driver/header-search-duplicates.c
 create mode 100644 clang/test/Driver/microsoft-header-search-duplicates.c

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1da8c82d52e618..03c27342f31f66 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -638,6 +638,20 @@ 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 for 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. The MSVC behavior is that user search
+  paths are ordered before system search paths (like gcc), and search paths that
+  duplicate an earlier search path are ignored regardless of user/system kind
+  (similar to gcc, but without the preference for system search paths over
+  duplicate user search paths). These differences are observable for driver
+  invocations that pass, e.g., `-Idir1 -isystem dir2 -isystem dir1`. The gcc
+  behavior will search `dir2` before `dir1` and the MSVC behavior will search
+  `dir1` before `dir2`.
+
 LoongArch Support
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index 2218db15013d92..3f487f3a4c1c05 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -368,7 +368,8 @@ void InitHeaderSearch::AddDefaultIncludePaths(
 /// If there are duplicate directory entries in the specified search list,
 /// remove the later (dead) ones.  Returns the number of non-system headers
 /// removed, which is used to update NumAngled.
-static unsigned RemoveDuplicates(std::vector<DirectoryLookupInfo> &SearchList,
+static unsigned RemoveDuplicates(const LangOptions &Lang,
+                                 std::vector<DirectoryLookupInfo> &SearchList,
                                  unsigned First, bool Verbose) {
   llvm::SmallPtrSet<const DirectoryEntry *, 8> SeenDirs;
   llvm::SmallPtrSet<const DirectoryEntry *, 8> SeenFrameworkDirs;
@@ -394,14 +395,15 @@ static unsigned RemoveDuplicates(std::vector<DirectoryLookupInfo> &SearchList,
         continue;
     }
 
-    // If we have a normal #include dir/framework/headermap that is shadowed
-    // later in the chain by a system include location, we actually want to
-    // ignore the user's request and drop the user dir... keeping the system
-    // dir.  This is weird, but required to emulate GCC's search path correctly.
+    // When not in MSVC compatibility mode, if we have a normal
+    // #include dir/framework/headermap that is shadowed later in the chain by
+    // a system include location, we actually want to ignore the user's request
+    // and drop the user dir... keeping the system dir.  This is weird, but
+    // required to emulate GCC's search path correctly.
     //
     // Since dupes of system dirs are rare, just rescan to find the original
     // that we're nuking instead of using a DenseMap.
-    if (CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
+    if (!Lang.MSVCCompat && CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
       // Find the dir that this is the same of.
       unsigned FirstDir;
       for (FirstDir = First;; ++FirstDir) {
@@ -484,14 +486,14 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
       SearchList.push_back(Include);
 
   // Deduplicate and remember index.
-  RemoveDuplicates(SearchList, 0, Verbose);
+  RemoveDuplicates(Lang, SearchList, 0, Verbose);
   unsigned NumQuoted = SearchList.size();
 
   for (auto &Include : IncludePath)
     if (Include.Group == Angled || Include.Group == IndexHeaderMap)
       SearchList.push_back(Include);
 
-  RemoveDuplicates(SearchList, NumQuoted, Verbose);
+  RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
   unsigned NumAngled = SearchList.size();
 
   for (auto &Include : IncludePath)
@@ -510,7 +512,8 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   // Remove duplicates across both the Angled and System directories.  GCC does
   // this and failing to remove duplicates across these two groups breaks
   // #include_next.
-  unsigned NonSystemRemoved = RemoveDuplicates(SearchList, NumQuoted, Verbose);
+  unsigned NonSystemRemoved =
+      RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
   NumAngled -= NonSystemRemoved;
 
   Headers.SetSearchPaths(extractLookups(SearchList), NumQuoted, NumAngled,
diff --git a/clang/test/Driver/header-search-duplicates.c b/clang/test/Driver/header-search-duplicates.c
new file mode 100644
index 00000000000000..ebd49453a1c10c
--- /dev/null
+++ b/clang/test/Driver/header-search-duplicates.c
@@ -0,0 +1,76 @@
+// Test that the gcc-like driver, when not in Microsoft compatibility mode,
+// emulates the gcc behavior of eliding a user header search path when the
+// same path is present as a system header search path.
+// See microsoft-header-search-duplicates.c for Microsoft compatible behavior.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Test the clang driver with a target that does not implicitly enable the
+// -fms-compatibility option. The -nostdinc option is used to suppress default
+// search paths to ease testing.
+// RUN: %clang \
+// RUN:     -target x86_64-unknown-linux-gnu \
+// RUN:     -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -I%t/include/w \
+// RUN:     -isystem %t/include/z \
+// RUN:     -I%t/include/x \
+// RUN:     -isystem %t/include/y \
+// RUN:     -isystem %t/include/x \
+// RUN:     -I%t/include/w \
+// RUN:     -isystem %t/include/y \
+// RUN:     -isystem %t/include/z \
+// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
+
+#--- test.c
+#include <a.h>
+#include <b.h>
+#include <c.h>
+
+// The expected behavior is that user search paths are ordered before system
+// search paths, that user search paths that duplicate a (later) system search
+// path are ignored, and that search paths that duplicate an earlier search
+// path of the same user/system kind are ignored.
+// CHECK:      ignoring duplicate directory "[[PWD]]/include/w"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
+// CHECK:      #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/include/w
+// CHECK-NEXT: [[PWD]]/include/z
+// CHECK-NEXT: [[PWD]]/include/y
+// CHECK-NEXT: [[PWD]]/include/x
+// CHECK-NEXT: End of search list.
+
+#--- include/w/b.h
+#define INCLUDE_W_B_H
+#include_next <b.h>
+
+#--- include/w/c.h
+#define INCLUDE_W_C_H
+#include_next <c.h>
+
+#--- include/x/a.h
+#if !defined(INCLUDE_Y_A_H)
+#error 'include/y/a.h' should have been included before 'include/x/a.h'!
+#endif
+
+#--- include/x/b.h
+#if !defined(INCLUDE_W_B_H)
+#error 'include/w/b.h' should have been included before 'include/x/b.h'!
+#endif
+
+#--- include/x/c.h
+#if !defined(INCLUDE_Z_C_H)
+#error 'include/z/c.h' should have been included before 'include/x/c.h'!
+#endif
+
+#--- include/y/a.h
+#define INCLUDE_Y_A_H
+#include_next <a.h>
+
+#--- include/z/c.h
+#define INCLUDE_Z_C_H
+#include_next <c.h>
diff --git a/clang/test/Driver/microsoft-header-search-duplicates.c b/clang/test/Driver/microsoft-header-search-duplicates.c
new file mode 100644
index 00000000000000..0ab3e88b951c5c
--- /dev/null
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -0,0 +1,97 @@
+// Test that the cl-like driver and the gcc-like driver, when in Microsoft
+// compatibility mode, retain user header search paths that are duplicated in
+// system header search paths.
+// See header-search-duplicates.c for gcc compatible behavior.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Test the clang driver with a Windows target that implicitly enables the
+// -fms-compatibility option. The -nostdinc option is used to suppress default
+// search paths to ease testing.
+// RUN: %clang \
+// RUN:     -target x86_64-pc-windows \
+// RUN:     -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -I%t/include/w \
+// RUN:     -isystem %t/include/z \
+// RUN:     -I%t/include/x \
+// RUN:     -isystem %t/include/y \
+// RUN:     -isystem %t/include/x \
+// RUN:     -I%t/include/w \
+// RUN:     -isystem %t/include/y \
+// RUN:     -isystem %t/include/z \
+// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
+
+// Test the clang-cl driver with a Windows target that implicitly enables the
+// -fms-compatibility option. The /X option is used instead of -nostdinc
+// because the latter option suppresses all system include paths including
+// those specified by -imsvc. The -nobuiltininc option is uesd to suppress
+// the Clang resource directory. The -nostdlibinc option is used to suppress
+// search paths for the Windows SDK, CRT, MFC, ATL, etc...
+// RUN: %clang_cl \
+// RUN:     -target x86_64-pc-windows \
+// RUN:     -v -fsyntax-only \
+// RUN:     /X \
+// RUN:     -nobuiltininc \
+// RUN:     -nostdlibinc \
+// RUN:     -I%t/include/w \
+// RUN:     -imsvc %t/include/z \
+// RUN:     -I%t/include/x \
+// RUN:     -imsvc %t/include/y \
+// RUN:     -imsvc %t/include/x \
+// RUN:     -I%t/include/w \
+// RUN:     -imsvc %t/include/y \
+// RUN:     -imsvc %t/include/z \
+// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
+
+#--- test.c
+#include <a.h>
+#include <b.h>
+#include <c.h>
+
+// The expected behavior is that user search paths are ordered before system
+// search paths and that search paths that duplicate an earlier search path
+// (regardless of user/system concerns) are ignored.
+// CHECK:      ignoring duplicate directory "[[PWD]]/include/w"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
+// CHECK-NOT:   as it is a non-system directory that duplicates a system directory
+// CHECK:      #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/include/w
+// CHECK-NEXT: [[PWD]]/include/x
+// CHECK-NEXT: [[PWD]]/include/z
+// CHECK-NEXT: [[PWD]]/include/y
+// CHECK-NEXT: End of search list.
+
+#--- include/w/b.h
+#define INCLUDE_W_B_H
+#include_next <b.h>
+
+#--- include/w/c.h
+#define INCLUDE_W_C_H
+#include_next <c.h>
+
+#--- include/x/a.h
+#define INCLUDE_X_A_H
+#include_next <a.h>
+
+#--- include/x/b.h
+#if !defined(INCLUDE_W_B_H)
+#error 'include/w/b.h' should have been included before 'include/x/b.h'!
+#endif
+
+#--- include/x/c.h
+#define INCLUDE_X_C_H
+
+#--- include/y/a.h
+#if !defined(INCLUDE_X_A_H)
+#error 'include/x/a.h' should have been included before 'include/y/a.h'!
+#endif
+
+#--- include/z/c.h
+#include_next <c.h>
+#if !defined(INCLUDE_X_C_H)
+#error 'include/x/c.h' should have been included before 'include/z/c.h'!
+#endif

>From 7c8f4eae75536572af819a01cfd4891de8e0e7fb Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Mon, 9 Sep 2024 11:49:29 -0700
Subject: [PATCH 2/9] WIP

---
 clang/include/clang/Driver/Options.td         |   5 +-
 clang/include/clang/Lex/HeaderSearchOptions.h |   4 +
 clang/lib/Driver/Driver.cpp                   |   3 +-
 clang/lib/Driver/ToolChains/Clang.cpp         |   3 +-
 clang/lib/Driver/ToolChains/MSVC.cpp          |   2 +
 clang/lib/Frontend/CompilerInvocation.cpp     |  26 +-
 clang/lib/Lex/InitHeaderSearch.cpp            |  64 ++--
 clang/test/Driver/header-search-duplicates.c  | 116 ++++--
 .../microsoft-header-search-duplicates.c      | 341 +++++++++++++++---
 llvm/include/llvm/Option/ArgList.h            |   3 +-
 llvm/lib/Option/ArgList.cpp                   |   5 +-
 11 files changed, 450 insertions(+), 122 deletions(-)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 379e75b197cf96..6e5aba2804302c 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4563,6 +4563,9 @@ 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 : JoinedOrSeparate<["-"], "iexternal">, Group<clang_i_Group>,
+  Visibility<[ClangOption, CC1Option]>,
+  HelpText<"Add directory to external include search path">, MetaVarName<"<directory>">;
 def iframework : JoinedOrSeparate<["-"], "iframework">, Group<clang_i_Group>,
   Visibility<[ClangOption, CC1Option]>,
   HelpText<"Add directory to SYSTEM framework search path">;
@@ -8426,7 +8429,7 @@ 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_fp_contract : CLFlag<"fp:contract">, HelpText<"">, Alias<ffp_contract>, AliasArgs<["on"]>;
diff --git a/clang/include/clang/Lex/HeaderSearchOptions.h b/clang/include/clang/Lex/HeaderSearchOptions.h
index 83a95e9ad90a7f..58893792cb4b77 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -38,6 +38,10 @@ 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.
+  External,
+
   /// Like Angled, but marks system directories.
   System,
 
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 9878a9dad78d40..a8a5f0a4f445e2 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1298,7 +1298,8 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
 
   // Check for missing include directories.
   if (!Diags.isIgnored(diag::warn_missing_include_dirs, SourceLocation())) {
-    for (auto IncludeDir : Args.getAllArgValues(options::OPT_I_Group)) {
+    for (auto IncludeDir :
+        Args.getAllArgValues(options::OPT_I_Group, options::OPT_iexternal)) {
       if (!VFS->exists(IncludeDir))
         Diag(diag::warn_missing_include_dirs) << IncludeDir;
     }
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..aca75494a848c6 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1185,7 +1185,8 @@ 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_embed_dir_EQ,
+                   options::OPT_iexternal});
 
   // Add -Wp, and -Xpreprocessor if using the preprocessor.
 
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 80799d1e715f07..b453e406e435f5 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -660,6 +660,8 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
     return false;
   };
 
+  // FIXME: /external:env won't cause user paths already present in the
+  // FIXME: search path to be dropped like /external:I does.
   // Add %INCLUDE%-like dirs via /external:env: flags.
   for (const auto &Var :
        DriverArgs.getAllArgValues(options::OPT__SLASH_external_env)) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index db7c791059a32e..4de73e0889fcdc 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3194,8 +3194,10 @@ 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 MSVC /external:I options
+  // in order.
+  for (; It < End && Matches(*It, {frontend::IndexHeaderMap, frontend::Angled,
+                                   frontend::External},
                              std::nullopt, true);
        ++It) {
     OptSpecifier Opt = [It, Matches]() {
@@ -3207,13 +3209,15 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
         return OPT_F;
       if (Matches(*It, frontend::Angled, false, true))
         return OPT_I;
+      if (Matches(*It, frontend::External, std::nullopt, 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);
-  };
+  }
 
   // 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
@@ -3323,8 +3327,8 @@ 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;
+  // Add the -I..., -F..., -index-header-map, and MSVC /external:I options
+  // options in order.
   bool IsSysrootSpecified =
       Args.hasArg(OPT__sysroot_EQ) || Args.hasArg(OPT_isysroot);
 
@@ -3343,15 +3347,19 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
     return A->getValue();
   };
 
-  for (const auto *A : Args.filtered(OPT_I, OPT_F, OPT_index_header_map)) {
+  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,
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index 3f487f3a4c1c05..d4f3bb2247a01d 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -366,8 +366,8 @@ void InitHeaderSearch::AddDefaultIncludePaths(
 }
 
 /// If there are duplicate directory entries in the specified search list,
-/// remove the later (dead) ones.  Returns the number of non-system headers
-/// removed, which is used to update NumAngled.
+/// identify and remove the ones to be ignored and issue a diagnostic.
+/// Returns the number of non-system search paths emoved.
 static unsigned RemoveDuplicates(const LangOptions &Lang,
                                  std::vector<DirectoryLookupInfo> &SearchList,
                                  unsigned First, bool Verbose) {
@@ -377,41 +377,43 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
   unsigned NonSystemRemoved = 0;
   for (unsigned i = First; i != SearchList.size(); ++i) {
     unsigned DirToRemove = i;
+    bool NonSystemDirRemoved = false;
 
     const DirectoryLookup &CurEntry = SearchList[i].Lookup;
 
+    // If the current entry is for a previously unseen location, cache it and
+    // continue with the next entry.
     if (CurEntry.isNormalDir()) {
-      // If this isn't the first time we've seen this dir, remove it.
       if (SeenDirs.insert(CurEntry.getDir()).second)
         continue;
     } else if (CurEntry.isFramework()) {
-      // If this isn't the first time we've seen this framework dir, remove it.
       if (SeenFrameworkDirs.insert(CurEntry.getFrameworkDir()).second)
         continue;
     } else {
       assert(CurEntry.isHeaderMap() && "Not a headermap or normal dir?");
-      // If this isn't the first time we've seen this headermap, remove it.
       if (SeenHeaderMaps.insert(CurEntry.getHeaderMap()).second)
         continue;
     }
 
-    // When not in MSVC compatibility mode, if we have a normal
-    // #include dir/framework/headermap that is shadowed later in the chain by
-    // a system include location, we actually want to ignore the user's request
-    // and drop the user dir... keeping the system dir.  This is weird, but
-    // required to emulate GCC's search path correctly.
+    // Pruning of duplicate search locations is intended to emulate the behavior
+    // exhibited by GCC (by default) or MSVC (in Microsoft compatibility mode)
+    // to ensure that #include and #include_next directives produce the same
+    // results as these other compilers.
     //
-    // Since dupes of system dirs are rare, just rescan to find the original
-    // that we're nuking instead of using a DenseMap.
-    if (!Lang.MSVCCompat && CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
-      // Find the dir that this is the same of.
+    // GCC and MSVC both prune duplicate user search locations that follow a
+    // previous matching user search location. Both compilers also prune user
+    // search locations that are also present as system search locations
+    // regardless of the order in which they appear. The compilers differ with
+    // respect to pruning system search locations that duplicate a previous
+    // system search location; GCC preserves the first such occurences while
+    // MSVC preserves the last one.
+    if (CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
+      // Find the matching search entry.
       unsigned FirstDir;
-      for (FirstDir = First;; ++FirstDir) {
-        assert(FirstDir != i && "Didn't find dupe?");
-
+      for (FirstDir = First; FirstDir < i; ++FirstDir) {
         const DirectoryLookup &SearchEntry = SearchList[FirstDir].Lookup;
 
-        // If these are different lookup types, then they can't be the dupe.
+        // Different lookup types are not considered duplicate entries.
         if (SearchEntry.getLookupType() != CurEntry.getLookupType())
           continue;
 
@@ -428,21 +430,34 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
         if (isSame)
           break;
       }
+      assert(FirstDir < i && "Expected duplicate search location not found");
 
-      // If the first dir in the search path is a non-system dir, zap it
-      // instead of the system one.
-      if (SearchList[FirstDir].Lookup.getDirCharacteristic() == SrcMgr::C_User)
+      if (Lang.MSVCCompat) {
+        // In Microsoft compatibility mode, a later system search location entry
+        // suppresses a previous user or system search location.
         DirToRemove = FirstDir;
+        if (SearchList[FirstDir].Lookup.getDirCharacteristic() ==
+            SrcMgr::C_User)
+          NonSystemDirRemoved = true;
+      } else {
+        // In GCC compatibility mode, a later system search location entry
+        // suppresses a previous user search location.
+        if (SearchList[FirstDir].Lookup.getDirCharacteristic() ==
+            SrcMgr::C_User) {
+          DirToRemove = FirstDir;
+          NonSystemDirRemoved = true;
+        }
+      }
     }
 
     if (Verbose) {
       llvm::errs() << "ignoring duplicate directory \""
                    << CurEntry.getName() << "\"\n";
-      if (DirToRemove != i)
+      if (NonSystemDirRemoved)
         llvm::errs() << "  as it is a non-system directory that duplicates "
                      << "a system directory\n";
     }
-    if (DirToRemove != i)
+    if (NonSystemDirRemoved)
       ++NonSystemRemoved;
 
     // This is reached if the current entry is a duplicate.  Remove the
@@ -490,7 +505,8 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   unsigned NumQuoted = SearchList.size();
 
   for (auto &Include : IncludePath)
-    if (Include.Group == Angled || Include.Group == IndexHeaderMap)
+    if (Include.Group == Angled || Include.Group == IndexHeaderMap ||
+        Include.Group == External)
       SearchList.push_back(Include);
 
   RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
diff --git a/clang/test/Driver/header-search-duplicates.c b/clang/test/Driver/header-search-duplicates.c
index ebd49453a1c10c..8ce50a8455ab40 100644
--- a/clang/test/Driver/header-search-duplicates.c
+++ b/clang/test/Driver/header-search-duplicates.c
@@ -1,6 +1,5 @@
-// Test that the gcc-like driver, when not in Microsoft compatibility mode,
-// emulates the gcc behavior of eliding a user header search path when the
-// same path is present as a system header search path.
+// Test that pruning of header search paths emulates GCC behavior when not in
+// Microsoft compatibility mode.
 // See microsoft-header-search-duplicates.c for Microsoft compatible behavior.
 
 // RUN: rm -rf %t
@@ -13,64 +12,123 @@
 // RUN:     -target x86_64-unknown-linux-gnu \
 // RUN:     -v -fsyntax-only \
 // RUN:     -nostdinc \
-// RUN:     -I%t/include/w \
-// RUN:     -isystem %t/include/z \
-// RUN:     -I%t/include/x \
-// RUN:     -isystem %t/include/y \
+// RUN:     -isystem %t/include/S \
+// RUN:     -isystem %t/include/w \
+// RUN:     -I%t/include/v \
 // RUN:     -isystem %t/include/x \
-// RUN:     -I%t/include/w \
+// RUN:     -I%t/include/z \
 // RUN:     -isystem %t/include/y \
 // RUN:     -isystem %t/include/z \
+// RUN:     -I%t/include/x \
+// RUN:     -isystem %t/include/y \
+// RUN:     -I%t/include/v \
+// RUN:     -isystem %t/include/w \
+// RUN:     -I%t/include/U \
 // RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
 
 #--- test.c
 #include <a.h>
 #include <b.h>
 #include <c.h>
+#include <d.h>
+#include <e.h>
+#include <f.h>
+#include <g.h>
 
 // The expected behavior is that user search paths are ordered before system
 // search paths, that user search paths that duplicate a (later) system search
 // path are ignored, and that search paths that duplicate an earlier search
 // path of the same user/system kind are ignored.
-// CHECK:      ignoring duplicate directory "[[PWD]]/include/w"
+// CHECK:      ignoring duplicate directory "[[PWD]]/include/v"
 // CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
 // CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
 // CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
 // CHECK:      #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/include/v
+// CHECK-NEXT: [[PWD]]/include/U
+// CHECK-NEXT: [[PWD]]/include/S
 // CHECK-NEXT: [[PWD]]/include/w
-// CHECK-NEXT: [[PWD]]/include/z
-// CHECK-NEXT: [[PWD]]/include/y
 // CHECK-NEXT: [[PWD]]/include/x
+// CHECK-NEXT: [[PWD]]/include/y
+// CHECK-NEXT: [[PWD]]/include/z
 // CHECK-NEXT: End of search list.
 
+#--- include/v/a.h
+
+#--- include/U/a.h
+#error 'include/U/a.h' should not have been included!
+
+#--- include/U/b.h
+
+#--- include/S/a.h
+#error 'include/U/a.h' should not have been included!
+
+#--- include/S/b.h
+#error 'include/S/b.h' should not have been included!
+
+#--- include/S/c.h
+
+#--- include/w/a.h
+#error 'include/w/a.h' should not have been included!
+
 #--- include/w/b.h
-#define INCLUDE_W_B_H
-#include_next <b.h>
+#error 'include/w/b.h' should not have been included!
 
 #--- include/w/c.h
-#define INCLUDE_W_C_H
-#include_next <c.h>
+#error 'include/w/c.h' should not have been included!
+
+#--- include/w/d.h
 
 #--- include/x/a.h
-#if !defined(INCLUDE_Y_A_H)
-#error 'include/y/a.h' should have been included before 'include/x/a.h'!
-#endif
+#error 'include/x/a.h' should not have been included!
 
 #--- include/x/b.h
-#if !defined(INCLUDE_W_B_H)
-#error 'include/w/b.h' should have been included before 'include/x/b.h'!
-#endif
+#error 'include/x/b.h' should not have been included!
 
 #--- include/x/c.h
-#if !defined(INCLUDE_Z_C_H)
-#error 'include/z/c.h' should have been included before 'include/x/c.h'!
-#endif
+#error 'include/x/c.h' should not have been included!
+
+#--- include/x/d.h
+#error 'include/x/d.h' should not have been included!
+
+#--- include/x/e.h
 
 #--- include/y/a.h
-#define INCLUDE_Y_A_H
-#include_next <a.h>
+#error 'include/y/a.h' should not have been included!
+
+#--- include/y/b.h
+#error 'include/y/b.h' should not have been included!
+
+#--- include/y/c.h
+#error 'include/y/c.h' should not have been included!
+
+#--- include/y/d.h
+#error 'include/y/d.h' should not have been included!
+
+#--- include/y/e.h
+#error 'include/y/e.h' should not have been included!
+
+#--- include/y/f.h
+
+#--- include/z/a.h
+#error 'include/z/a.h' should not have been included!
+
+#--- include/z/b.h
+#error 'include/z/b.h' should not have been included!
 
 #--- include/z/c.h
-#define INCLUDE_Z_C_H
-#include_next <c.h>
+#error 'include/z/c.h' should not have been included!
+
+#--- include/z/d.h
+#error 'include/z/d.h' should not have been included!
+
+#--- include/z/e.h
+#error 'include/z/e.h' should not have been included!
+
+#--- include/z/f.h
+#error 'include/z/f.h' should not have been included!
+
+#--- include/z/g.h
diff --git a/clang/test/Driver/microsoft-header-search-duplicates.c b/clang/test/Driver/microsoft-header-search-duplicates.c
index 0ab3e88b951c5c..3a01df8e90e4f4 100644
--- a/clang/test/Driver/microsoft-header-search-duplicates.c
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -1,7 +1,6 @@
-// Test that the cl-like driver and the gcc-like driver, when in Microsoft
-// compatibility mode, retain user header search paths that are duplicated in
-// system header search paths.
-// See header-search-duplicates.c for gcc compatible behavior.
+// Test that pruning of header search paths emulates MSVC behavior when in
+// Microsoft compatibility mode.
+// See header-search-duplicates.c for GCC compatible behavior.
 
 // RUN: rm -rf %t
 // RUN: split-file %s %t
@@ -9,89 +8,323 @@
 // Test the clang driver with a Windows target that implicitly enables the
 // -fms-compatibility option. The -nostdinc option is used to suppress default
 // search paths to ease testing.
-// RUN: %clang \
-// RUN:     -target x86_64-pc-windows \
-// RUN:     -v -fsyntax-only \
-// RUN:     -nostdinc \
-// RUN:     -I%t/include/w \
-// RUN:     -isystem %t/include/z \
-// RUN:     -I%t/include/x \
-// RUN:     -isystem %t/include/y \
-// RUN:     -isystem %t/include/x \
-// RUN:     -I%t/include/w \
-// RUN:     -isystem %t/include/y \
-// RUN:     -isystem %t/include/z \
-// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
+// XUN: env INCLUDE="%t/include/p;%t/include/o" \
+// XUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
+// XUN  %clang \
+// XUN:     -target x86_64-pc-windows \
+// XUN:     -v -fsyntax-only \
+// XUN:     -nostdinc \
+// XUN:     -isystem %t/include/s \
+// XUN:     -isystem %t/include/w \
+// XUN:     -I%t/include/v \
+// XUN:     -isystem %t/include/x \
+// XUN:     -I%t/include/z \
+// XUN:     -isystem %t/include/y \
+// XUN:     -I%t/include/r \
+// XUN:     -external:env:EXTRA_INCLUDE \
+// XUN:     -I%t/include/t \
+// XUN:     -isystem %t/include/z \
+// XUN:     -I%t/include/o \
+// XUN:     -I%t/include/x \
+// XUN:     -isystem %t/include/y \
+// XUN:     -I%t/include/v \
+// XUN:     -isystem %t/include/w \
+// XUN:     -I%t/include/u \
+// XUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
 
 // Test the clang-cl driver with a Windows target that implicitly enables the
 // -fms-compatibility option. The /X option is used instead of -nostdinc
 // because the latter option suppresses all system include paths including
-// those specified by -imsvc. The -nobuiltininc option is uesd to suppress
+// those specified by /external:I. The -nobuiltininc option is uesd to suppress
 // the Clang resource directory. The -nostdlibinc option is used to suppress
 // search paths for the Windows SDK, CRT, MFC, ATL, etc...
+// RUN: env INCLUDE="%t/include/p;%t/include/o" \
+// RUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
 // RUN: %clang_cl \
 // RUN:     -target x86_64-pc-windows \
 // RUN:     -v -fsyntax-only \
-// RUN:     /X \
 // RUN:     -nobuiltininc \
 // RUN:     -nostdlibinc \
-// RUN:     -I%t/include/w \
-// RUN:     -imsvc %t/include/z \
-// RUN:     -I%t/include/x \
-// RUN:     -imsvc %t/include/y \
-// RUN:     -imsvc %t/include/x \
-// RUN:     -I%t/include/w \
-// RUN:     -imsvc %t/include/y \
-// RUN:     -imsvc %t/include/z \
+// RUN:     /external:I %t/include/s \
+// RUN:     /external:I %t/include/w \
+// RUN:     /I%t/include/v \
+// RUN:     /external:I %t/include/x \
+// RUN:     /I%t/include/z \
+// RUN:     /external:I %t/include/y \
+// RUN:     /I%t/include/r \
+// RUN:     /external:env:EXTRA_INCLUDE \
+// RUN:     /I%t/include/t \
+// RUN:     /external:I %t/include/z \
+// RUN:     /I%t/include/o \
+// RUN:     /I%t/include/x \
+// RUN:     /external:I %t/include/y \
+// RUN:     /I%t/include/v \
+// RUN:     /external:I %t/include/w \
+// RUN:     /I%t/include/u \
 // RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
 
 #--- test.c
 #include <a.h>
 #include <b.h>
 #include <c.h>
+#include <d.h>
+#include <e.h>
+#include <f.h>
+#include <g.h>
+#include <h.h>
+#include <i.h>
+#include <j.h>
+#include <k.h>
+#include <l.h>
 
-// The expected behavior is that user search paths are ordered before system
-// search paths and that search paths that duplicate an earlier search path
-// (regardless of user/system concerns) are ignored.
-// CHECK:      ignoring duplicate directory "[[PWD]]/include/w"
+// The expected behavior is that user search paths that duplicate a system search
+// path are ignored, that user search paths that duplicate a previous user
+// search path are ignored, and that system search search paths that duplicate
+// a later system search path are ignored.
+// CHECK:      ignoring duplicate directory "[[PWD]]/include/v"
 // CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
 // CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
-// CHECK-NOT:   as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/t"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/r"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
 // CHECK:      #include <...> search starts here:
-// CHECK-NEXT: [[PWD]]/include/w
+// CHECK-NEXT: [[PWD]]/include/s
+// CHECK-NEXT: [[PWD]]/include/v
 // CHECK-NEXT: [[PWD]]/include/x
+// CHECK-NEXT: [[PWD]]/include/r
+// CHECK-NEXT: [[PWD]]/include/t
 // CHECK-NEXT: [[PWD]]/include/z
+// CHECK-NEXT: [[PWD]]/include/o
 // CHECK-NEXT: [[PWD]]/include/y
+// CHECK-NEXT: [[PWD]]/include/w
+// CHECK-NEXT: [[PWD]]/include/u
+// CHECK-NEXT: [[PWD]]/include/q
+// CHECK-NEXT: [[PWD]]/include/p
 // CHECK-NEXT: End of search list.
 
-#--- include/w/b.h
-#define INCLUDE_W_B_H
-#include_next <b.h>
+#--- include/s/a.h
 
-#--- include/w/c.h
-#define INCLUDE_W_C_H
-#include_next <c.h>
+#--- include/v/a.h
+#error 'include/v/a.h' should not have been included!
+
+#--- include/v/b.h
 
 #--- include/x/a.h
-#define INCLUDE_X_A_H
-#include_next <a.h>
+#error 'include/x/a.h' should not have been included!
 
 #--- include/x/b.h
-#if !defined(INCLUDE_W_B_H)
-#error 'include/w/b.h' should have been included before 'include/x/b.h'!
-#endif
+#error 'include/x/b.h' should not have been included!
 
 #--- include/x/c.h
-#define INCLUDE_X_C_H
 
-#--- include/y/a.h
-#if !defined(INCLUDE_X_A_H)
-#error 'include/x/a.h' should have been included before 'include/y/a.h'!
-#endif
+#--- include/r/a.h
+#error 'include/r/a.h' should not have been included!
+
+#--- include/r/b.h
+#error 'include/r/b.h' should not have been included!
+
+#--- include/r/c.h
+#error 'include/r/c.h' should not have been included!
+
+#--- include/r/d.h
+
+#--- include/t/a.h
+#error 'include/t/a.h' should not have been included!
+
+#--- include/t/b.h
+#error 'include/t/b.h' should not have been included!
+
+#--- include/t/c.h
+#error 'include/t/c.h' should not have been included!
+
+#--- include/t/d.h
+#error 'include/t/cdh' should not have been included!
+
+#--- include/t/e.h
+
+#--- include/z/a.h
+#error 'include/z/a.h' should not have been included!
+
+#--- include/z/b.h
+#error 'include/z/b.h' should not have been included!
 
 #--- include/z/c.h
-#include_next <c.h>
-#if !defined(INCLUDE_X_C_H)
-#error 'include/x/c.h' should have been included before 'include/z/c.h'!
-#endif
+#error 'include/z/c.h' should not have been included!
+
+#--- include/z/d.h
+#error 'include/z/d.h' should not have been included!
+
+#--- include/z/e.h
+#error 'include/z/e.h' should not have been included!
+
+#--- include/z/f.h
+
+#--- include/o/a.h
+#error 'include/o/a.h' should not have been included!
+
+#--- include/o/b.h
+#error 'include/o/b.h' should not have been included!
+
+#--- include/o/c.h
+#error 'include/o/c.h' should not have been included!
+
+#--- include/o/d.h
+#error 'include/o/d.h' should not have been included!
+
+#--- include/o/e.h
+#error 'include/o/e.h' should not have been included!
+
+#--- include/o/f.h
+#error 'include/o/f.h' should not have been included!
+
+#--- include/o/g.h
+
+#--- include/y/a.h
+#error 'include/y/a.h' should not have been included!
+
+#--- include/y/b.h
+#error 'include/y/b.h' should not have been included!
+
+#--- include/y/c.h
+#error 'include/y/c.h' should not have been included!
+
+#--- include/y/d.h
+#error 'include/y/d.h' should not have been included!
+
+#--- include/y/e.h
+#error 'include/y/e.h' should not have been included!
+
+#--- include/y/f.h
+#error 'include/y/f.h' should not have been included!
+
+#--- include/y/g.h
+#error 'include/y/g.h' should not have been included!
+
+#--- include/y/h.h
+
+#--- include/w/a.h
+#error 'include/w/a.h' should not have been included!
+
+#--- include/w/b.h
+#error 'include/w/b.h' should not have been included!
+
+#--- include/w/c.h
+#error 'include/w/c.h' should not have been included!
+
+#--- include/w/d.h
+#error 'include/w/d.h' should not have been included!
+
+#--- include/w/e.h
+#error 'include/w/e.h' should not have been included!
+
+#--- include/w/f.h
+#error 'include/w/f.h' should not have been included!
+
+#--- include/w/g.h
+#error 'include/w/g.h' should not have been included!
+
+#--- include/w/h.h
+#error 'include/w/h.h' should not have been included!
+
+#--- include/w/i.h
+
+#--- include/u/a.h
+#error 'include/u/a.h' should not have been included!
+
+#--- include/u/b.h
+#error 'include/u/b.h' should not have been included!
+
+#--- include/u/c.h
+#error 'include/u/c.h' should not have been included!
+
+#--- include/u/d.h
+#error 'include/u/d.h' should not have been included!
+
+#--- include/u/e.h
+#error 'include/u/e.h' should not have been included!
+
+#--- include/u/f.h
+#error 'include/u/f.h' should not have been included!
+
+#--- include/u/g.h
+#error 'include/u/g.h' should not have been included!
+
+#--- include/u/h.h
+#error 'include/u/h.h' should not have been included!
+
+#--- include/u/i.h
+#error 'include/u/i.h' should not have been included!
+
+#--- include/u/j.h
+
+#--- include/q/a.h
+#error 'include/q/a.h' should not have been included!
+
+#--- include/q/b.h
+#error 'include/q/b.h' should not have been included!
+
+#--- include/q/c.h
+#error 'include/q/c.h' should not have been included!
+
+#--- include/q/d.h
+#error 'include/q/d.h' should not have been included!
+
+#--- include/q/e.h
+#error 'include/q/e.h' should not have been included!
+
+#--- include/q/f.h
+#error 'include/q/f.h' should not have been included!
+
+#--- include/q/g.h
+#error 'include/q/g.h' should not have been included!
+
+#--- include/q/h.h
+#error 'include/q/h.h' should not have been included!
+
+#--- include/q/i.h
+#error 'include/q/i.h' should not have been included!
+
+#--- include/q/j.h
+#error 'include/q/j.h' should not have been included!
+
+#--- include/q/k.h
+
+#--- include/p/a.h
+#error 'include/p/a.h' should not have been included!
+
+#--- include/p/b.h
+#error 'include/p/b.h' should not have been included!
+
+#--- include/p/c.h
+#error 'include/p/c.h' should not have been included!
+
+#--- include/p/d.h
+#error 'include/p/d.h' should not have been included!
+
+#--- include/p/e.h
+#error 'include/p/e.h' should not have been included!
+
+#--- include/p/f.h
+#error 'include/p/f.h' should not have been included!
+
+#--- include/p/g.h
+#error 'include/p/g.h' should not have been included!
+
+#--- include/p/h.h
+#error 'include/p/h.h' should not have been included!
+
+#--- include/p/i.h
+#error 'include/p/i.h' should not have been included!
+
+#--- include/p/j.h
+#error 'include/p/j.h' should not have been included!
+
+#--- include/p/k.h
+#error 'include/p/k.h' should not have been included!
+
+#--- include/p/l.h
diff --git a/llvm/include/llvm/Option/ArgList.h b/llvm/include/llvm/Option/ArgList.h
index 09812f976d0166..072a7a371d7a26 100644
--- a/llvm/include/llvm/Option/ArgList.h
+++ b/llvm/include/llvm/Option/ArgList.h
@@ -288,7 +288,8 @@ class ArgList {
 
   /// getAllArgValues - Get the values of all instances of the given argument
   /// as strings.
-  std::vector<std::string> getAllArgValues(OptSpecifier Id) const;
+  std::vector<std::string>
+  getAllArgValues(OptSpecifier Id0, OptSpecifier Id1 = 0U) const;
 
   /// @}
   /// @name Translation Utilities
diff --git a/llvm/lib/Option/ArgList.cpp b/llvm/lib/Option/ArgList.cpp
index 6e164150d2e5e9..345754dec9d7b9 100644
--- a/llvm/lib/Option/ArgList.cpp
+++ b/llvm/lib/Option/ArgList.cpp
@@ -95,9 +95,10 @@ StringRef ArgList::getLastArgValue(OptSpecifier Id, StringRef Default) const {
   return Default;
 }
 
-std::vector<std::string> ArgList::getAllArgValues(OptSpecifier Id) const {
+std::vector<std::string>
+ArgList::getAllArgValues(OptSpecifier Id0, OptSpecifier Id1) const {
   SmallVector<const char *, 16> Values;
-  AddAllArgValues(Values, Id);
+  AddAllArgValues(Values, Id0, Id1);
   return std::vector<std::string>(Values.begin(), Values.end());
 }
 

>From 70e165e1c8c01931ce5eb4647e408fee05cd5d67 Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Fri, 18 Oct 2024 20:56:33 -0700
Subject: [PATCH 3/9] WIP to be squashed.

This passes all tests, but:
- Cleanup is needed; comments and doc are not in sync with code.
- There are a number of FIXMEs.
- Should env vars be expanded by the driver? Or ok to pass to cc1?
- New tests need to be refined to not be a single monolithic impossible
  to understand monstrosity.
---
 clang/include/clang/Driver/Options.td         | 16 +++++--
 clang/include/clang/Driver/ToolChain.h        |  3 ++
 clang/include/clang/Lex/HeaderSearchOptions.h | 16 ++++++-
 clang/lib/Driver/Driver.cpp                   |  3 +-
 clang/lib/Driver/ToolChain.cpp                | 11 +++++
 clang/lib/Driver/ToolChains/Clang.cpp         |  3 +-
 clang/lib/Driver/ToolChains/MSVC.cpp          | 45 ++++++++---------
 clang/lib/Frontend/CompilerInvocation.cpp     | 29 +++++++++--
 clang/lib/Lex/InitHeaderSearch.cpp            | 42 ++++++++++++----
 clang/test/Driver/cl-include.c                | 32 +++++--------
 clang/test/Driver/cl-options.c                |  6 ++-
 .../microsoft-header-search-duplicates.c      | 48 +++++++++++--------
 llvm/include/llvm/Option/ArgList.h            |  7 ++-
 llvm/lib/Option/ArgList.cpp                   |  7 ---
 14 files changed, 170 insertions(+), 98 deletions(-)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 6e5aba2804302c..ec1f9199fff0de 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4563,9 +4563,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 : JoinedOrSeparate<["-"], "iexternal">, Group<clang_i_Group>,
+def iexternal : JoinedOrSeparate<["-"], "iexternal">, Group<I_Group>,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"Add directory to external include search path">, MetaVarName<"<directory>">;
+  HelpText<"Add directory to include search path with warnings suppressed">, MetaVarName<"<dir>">;
+def iexternal_after : JoinedOrSeparate<["-"], "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, CC1Option]>,
+  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">;
@@ -8432,6 +8438,9 @@ def _SLASH_E : CLFlag<"E">, HelpText<"Preprocess to stdout">, Alias<E>;
 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"]>;
@@ -8661,9 +8670,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">,
diff --git a/clang/include/clang/Driver/ToolChain.h b/clang/include/clang/Driver/ToolChain.h
index 5347e29be91439..bcac7e17eda881 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/clang/include/clang/Driver/ToolChain.h
@@ -239,6 +239,9 @@ class ToolChain {
   static void addSystemIncludes(const llvm::opt::ArgList &DriverArgs,
                                 llvm::opt::ArgStringList &CC1Args,
                                 ArrayRef<StringRef> Paths);
+  static void addExternalAfterIncludes(const llvm::opt::ArgList &DriverArgs,
+                                       llvm::opt::ArgStringList &CC1Args,
+                                       ArrayRef<StringRef> Paths);
 
   static std::string concat(StringRef Path, const Twine &A, const Twine &B = "",
                             const Twine &C = "", const Twine &D = "");
diff --git a/clang/include/clang/Lex/HeaderSearchOptions.h b/clang/include/clang/Lex/HeaderSearchOptions.h
index 58893792cb4b77..924a41e7c6fc64 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -38,10 +38,22 @@ 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.
+  /// Like Angled, but marks system-like directories while retaining relative
+  /// order with user directories.
+  // FIXME: External is intended to match the semantics of the MSVC /external:I
+  // FIXME: option. MSVC handles such paths similarly to system paths, but
+  // FIXME: allows for separate specification of diagnostic level via the
+  // FIXME: /external:Wn option. That option does not affect the diagnostic
+  // FIXME: level used for system paths (those present in the INCLUDE env var).
+  // FIXME: Supporting the diagnostic level controls will likely require adding
+  // FIXME: an additional C_External enumerator to SrcMgr::CharacteristicKind.
   External,
 
+  // Like External, but searched after other external directories but before
+  // system directories. This is intended to match the semantics of the MSVC
+  // /external:env option.
+  ExternalAfter,
+
   /// Like Angled, but marks system directories.
   System,
 
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index a8a5f0a4f445e2..af4104e73cd9de 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1299,7 +1299,8 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   // Check for missing include directories.
   if (!Diags.isIgnored(diag::warn_missing_include_dirs, SourceLocation())) {
     for (auto IncludeDir :
-        Args.getAllArgValues(options::OPT_I_Group, options::OPT_iexternal)) {
+        Args.getAllArgValues(options::OPT_I_Group, 
+                             options::OPT_iexternal_after)) {
       if (!VFS->exists(IncludeDir))
         Diag(diag::warn_missing_include_dirs) << IncludeDir;
     }
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 4df31770950858..3e83046189504d 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1265,6 +1265,17 @@ void ToolChain::addExternCSystemIncludeIfExists(const ArgList &DriverArgs,
   }
 }
 
+/// 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));
+  }
+}
+
 /*static*/ std::string ToolChain::concat(StringRef Path, const Twine &A,
                                          const Twine &B, const Twine &C,
                                          const Twine &D) {
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index aca75494a848c6..3fc39296f44281 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1185,8 +1185,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.
 
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index b453e406e435f5..410a61502629eb 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -644,30 +644,11 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                   "include");
   }
 
+  // FIXME: Why does the -imsvc option exist?
   // Add %INCLUDE%-like directories from the -imsvc flag.
   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;
-  };
-
-  // FIXME: /external:env won't cause user paths already present in the
-  // FIXME: search path to be dropped like /external:I does.
-  // 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)) {
@@ -684,14 +665,26 @@ 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 system paths from the %INCLUDE% environment variable if neither a
+  // vctoolsdir or winsysroot directory has been explicitly specified.
+  // If any paths are present in %INCLUDE%, skip adding additional system
+  // directories.
+  // FIXME: Paths specified by the INCLUDE environment variable should be
+  // FIXME: handled as system paths distinct from external paths added by
+  // FIXME: /external:I or /external:env since /external:Wn options do not
+  // FIXME: apply to these paths. These paths are currently mapped to
+  // FIXME: -iexternal-after to avoid include path ordering issues when
+  // FIXME: duplicate paths are present.
   if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
                              options::OPT__SLASH_winsysroot)) {
-    bool Found = AddSystemIncludesFromEnv("INCLUDE");
-    Found |= AddSystemIncludesFromEnv("EXTERNAL_INCLUDE");
-    if (Found)
-      return;
+    if (auto Val = llvm::sys::Process::GetEnv("INCLUDE")) {
+      SmallVector<StringRef, 8> Dirs;
+      StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+      if (!Dirs.empty()) {
+        addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
+        return;
+      }
+    }
   }
 
   // When built with access to the proper Windows APIs, try to actually find
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 4de73e0889fcdc..a2c3cfa151b617 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3209,7 +3209,7 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
         return OPT_F;
       if (Matches(*It, frontend::Angled, false, true))
         return OPT_I;
-      if (Matches(*It, frontend::External, std::nullopt, true))
+      if (Matches(*It, frontend::External, false, true))
         return OPT_iexternal;
       llvm_unreachable("Unexpected HeaderSearchOptions::Entry.");
     }();
@@ -3219,6 +3219,11 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
     GenerateArg(Consumer, Opt, It->Path);
   }
 
+  // Add the paths for the MSVC /external: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
   // position on command line was such that this has no semantic impact on
@@ -3327,8 +3332,6 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
         llvm::CachedHashString(MacroDef.split('=').first));
   }
 
-  // Add the -I..., -F..., -index-header-map, and MSVC /external:I options
-  // options in order.
   bool IsSysrootSpecified =
       Args.hasArg(OPT__sysroot_EQ) || Args.hasArg(OPT_isysroot);
 
@@ -3347,6 +3350,8 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
     return A->getValue();
   };
 
+  // Add the -I..., -F..., -index-header-map, and MSVC /external:I options
+  // options in order.
   bool IsIndexHeaderMap = false;
   for (const auto *A : Args.filtered(OPT_I, OPT_F, OPT_index_header_map,
                                      OPT_iexternal)) {
@@ -3367,6 +3372,24 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
     IsIndexHeaderMap = false;
   }
 
+  // Add the MSVC /external:env and -iexternal-after options in order.
+  for (const auto *A :
+      Args.filtered(OPT_iexternal_after, OPT_iexternal_env_EQ)) {
+    if (A->getOption().matches(OPT_iexternal_after))
+      Opts.AddPath(A->getValue(), frontend::ExternalAfter,
+                   /*IsFramework=*/false, /*IgnoreSysRoot=*/true);
+    else {
+      if (auto Val = llvm::sys::Process::GetEnv(A->getValue())) {
+        SmallVector<StringRef, 8> Dirs;
+        StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+        for (const auto &Dir : Dirs) {
+          Opts.AddPath(Dir, 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 :
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index d4f3bb2247a01d..ebe2b30d80ac15 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -154,6 +154,8 @@ bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group,
   } else if (Group == ExternCSystem) {
     Type = SrcMgr::C_ExternCSystem;
   } else {
+    // Group in External, ExternalAfter, System, (Obj)C(XX)System, After.
+    // FIXME: Assert that the group is known?
     Type = SrcMgr::C_System;
   }
 
@@ -367,10 +369,11 @@ void InitHeaderSearch::AddDefaultIncludePaths(
 
 /// If there are duplicate directory entries in the specified search list,
 /// identify and remove the ones to be ignored and issue a diagnostic.
-/// Returns the number of non-system search paths emoved.
+/// Returns the number of non-system search paths removed.
 static unsigned RemoveDuplicates(const LangOptions &Lang,
                                  std::vector<DirectoryLookupInfo> &SearchList,
-                                 unsigned First, bool Verbose) {
+                                 unsigned First, bool Verbose,
+                                 bool RemovePreviousAllowed = true) {
   llvm::SmallPtrSet<const DirectoryEntry *, 8> SeenDirs;
   llvm::SmallPtrSet<const DirectoryEntry *, 8> SeenFrameworkDirs;
   llvm::SmallPtrSet<const HeaderMap *, 8> SeenHeaderMaps;
@@ -405,9 +408,10 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
     // search locations that are also present as system search locations
     // regardless of the order in which they appear. The compilers differ with
     // respect to pruning system search locations that duplicate a previous
-    // system search location; GCC preserves the first such occurences while
+    // system search location; GCC preserves the first such occurence while
     // MSVC preserves the last one.
-    if (CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
+    if (RemovePreviousAllowed &&
+        CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
       // Find the matching search entry.
       unsigned FirstDir;
       for (FirstDir = First; FirstDir < i; ++FirstDir) {
@@ -495,23 +499,40 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   std::vector<DirectoryLookupInfo> SearchList;
   SearchList.reserve(IncludePath.size());
 
-  // Quoted arguments go first.
+  // Add search paths for quoted inclusion first.
   for (auto &Include : IncludePath)
     if (Include.Group == Quoted)
       SearchList.push_back(Include);
 
-  // Deduplicate and remember index.
+  // Remove duplicate search paths within the quoted inclusion list.
   RemoveDuplicates(Lang, SearchList, 0, Verbose);
   unsigned NumQuoted = SearchList.size();
 
+  // FIXME: GCC Ignore the last path for quoted inclusion when it matches the
+  // FIXME: first path for angled inclusion. This is observable when using
+  // FIXME: #include_next.
+
+  // Add search paths for angled inclusion next.
   for (auto &Include : IncludePath)
     if (Include.Group == Angled || Include.Group == IndexHeaderMap ||
         Include.Group == External)
       SearchList.push_back(Include);
 
+  // Remove duplicate search paths within the angled inclusion list.
+  // This may leave paths duplicated across the quoted and angled inclusion
+  // sections.
   RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
   unsigned NumAngled = SearchList.size();
 
+  // Add search paths for external-after inclusion next and remove duplicates
+  // within them.
+  for (auto &Include : IncludePath)
+    if (Include.Group == ExternalAfter)
+      SearchList.push_back(Include);
+  RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose,
+                   /*RemovePreviousAllowed*/ false);
+
+  // Add search paths for language dependent system paths next.
   for (auto &Include : IncludePath)
     if (Include.Group == System || Include.Group == ExternCSystem ||
         (!Lang.ObjC && !Lang.CPlusPlus && Include.Group == CSystem) ||
@@ -520,14 +541,15 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
         (Lang.ObjC && !Lang.CPlusPlus && Include.Group == ObjCSystem) ||
         (Lang.ObjC && Lang.CPlusPlus && Include.Group == ObjCXXSystem))
       SearchList.push_back(Include);
-
+  // Add search paths for system paths to be searched after other system paths.
   for (auto &Include : IncludePath)
     if (Include.Group == After)
       SearchList.push_back(Include);
 
-  // Remove duplicates across both the Angled and System directories.  GCC does
-  // this and failing to remove duplicates across these two groups breaks
-  // #include_next.
+  // Remove duplicate search paths across both the angled inclusion list and
+  // the system search paths. This duplicate removal is necessary to ensure
+  // that header lookup in #include_next directives doesn't resolve to the
+  // same file.
   unsigned NonSystemRemoved =
       RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
   NumAngled -= NonSystemRemoved;
diff --git a/clang/test/Driver/cl-include.c b/clang/test/Driver/cl-include.c
index ca9e7db1e6f07c..a4a6cb4335f08a 100644
--- a/clang/test/Driver/cl-include.c
+++ b/clang/test/Driver/cl-include.c
@@ -7,37 +7,31 @@
 // 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 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"
+// RUN: env INCLUDE=/my/system/inc %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=STDINC
+// STDINC: "-iexternal-after" "/my/system/inc"
 
 // -nostdinc suppresses all of %INCLUDE%, clang resource dirs, and -imsvc dirs.
-// 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
+// RUN: env INCLUDE=/my/system/inc %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: "-iexternal-after" "/my/system/inc"
 // NOSTDINC-NOT: "-internal-isystem" "{{.*lib.*clang.*include}}"
-// NOSTDINC-NOT: "-internal-isystem" "/my/other/inc"
 
-// /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
+// /X suppresses %INCLUDE% but not clang resource dirs, -imsvc dirs, or /external: flags.
+// RUN: env INCLUDE=/my/system/inc 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-NOT: "-external-after" "/my/system/inc"
+// SLASHX: "-iexternal-env=FOO"
 // 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
+// /winsysroot does not suppress -imsvc dirs or /external: flags.
+// RUN: 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: "-iexternal-env=FOO"
 // 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"
+// EXTERNAL_ENV: "-iexternal-env=FOO"
+// EXTERNAL_ENV: "-iexternal-env=BAR"
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index 8191fda97788c1..ec194cadae3d8a 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -40,7 +40,10 @@
 
 // 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"
+// EXTERNAL_I: "-iexternal" "path"
+
+// RUN: env EXTPATH="path1;path2" %clang_cl /external:env:EXTPATH -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_ENV %s
+// EXTERNAL_ENV: "-iexternal-env=EXTPATH"
 
 // RUN: %clang_cl /fp:fast /fp:except -### -- %s 2>&1 | FileCheck -check-prefix=fpexcept %s
 // fpexcept-NOT: -funsafe-math-optimizations
@@ -443,7 +446,6 @@
 // RUN:     /experimental:preprocessor \
 // RUN:     /exportHeader /headerName:foo \
 // RUN:     /external:anglebrackets \
-// RUN:     /external:env:var \
 // RUN:     /external:W0 \
 // RUN:     /external:W1 \
 // RUN:     /external:W2 \
diff --git a/clang/test/Driver/microsoft-header-search-duplicates.c b/clang/test/Driver/microsoft-header-search-duplicates.c
index 3a01df8e90e4f4..05157270590d36 100644
--- a/clang/test/Driver/microsoft-header-search-duplicates.c
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -33,11 +33,9 @@
 // XUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
 
 // Test the clang-cl driver with a Windows target that implicitly enables the
-// -fms-compatibility option. The /X option is used instead of -nostdinc
-// because the latter option suppresses all system include paths including
-// those specified by /external:I. The -nobuiltininc option is uesd to suppress
-// the Clang resource directory. The -nostdlibinc option is used to suppress
-// search paths for the Windows SDK, CRT, MFC, ATL, etc...
+// -fms-compatibility option. The -nobuiltininc option is uesd to suppress the
+// Clang resource directory. The -nostdlibinc option is used to suppress search
+// paths for the Windows SDK, CRT, MFC, ATL, etc...
 // RUN: env INCLUDE="%t/include/p;%t/include/o" \
 // RUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
 // RUN: %clang_cl \
@@ -77,21 +75,31 @@
 #include <k.h>
 #include <l.h>
 
-// The expected behavior is that user search paths that duplicate a system search
-// path are ignored, that user search paths that duplicate a previous user
-// search path are ignored, and that system search search paths that duplicate
-// a later system search path are ignored.
-// CHECK:      ignoring duplicate directory "[[PWD]]/include/v"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
-// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
-// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/t"
-// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/r"
-// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
+// Header search paths are processed as follows:
+// 1) Paths specified by the /I and /external:I options are processed in order.
+//    1.1) Paths specified by /I that duplicate a path specified by /external:I
+//         are ignored regardless of the option order.
+//    1.2) Paths specified by /I that duplicate a prior /I option are ignored.
+//    1.3) Paths specified by /external:I that duplicate a later /external:I
+//         option are ignored.
+// 2) Paths specified by the /external:env options are processed in order.
+//    Paths that duplicate a path from a /I option, an external:I option, a
+//    prior /external:env option, or a prior path from the current /external:env
+//    option are ignored.
+// 3) Paths specified by the %INCLUDE% environment variable are processed in
+//    order. Paths that duplicate any other path are ignored.
+
+// XHECK:      ignoring duplicate directory "[[PWD]]/include/v"
+// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
+// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
+// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/t"
+// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/r"
+// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
 // CHECK:      #include <...> search starts here:
 // CHECK-NEXT: [[PWD]]/include/s
 // CHECK-NEXT: [[PWD]]/include/v
diff --git a/llvm/include/llvm/Option/ArgList.h b/llvm/include/llvm/Option/ArgList.h
index 072a7a371d7a26..08d8e9d5b73d11 100644
--- a/llvm/include/llvm/Option/ArgList.h
+++ b/llvm/include/llvm/Option/ArgList.h
@@ -288,8 +288,13 @@ class ArgList {
 
   /// getAllArgValues - Get the values of all instances of the given argument
   /// as strings.
+  template<typename ...OptSpecifiers>
   std::vector<std::string>
-  getAllArgValues(OptSpecifier Id0, OptSpecifier Id1 = 0U) const;
+  getAllArgValues(OptSpecifiers ...Ids) const {
+    SmallVector<const char *, 16> Values;
+    AddAllArgValues(Values, Ids...);
+    return std::vector<std::string>(Values.begin(), Values.end());
+  }
 
   /// @}
   /// @name Translation Utilities
diff --git a/llvm/lib/Option/ArgList.cpp b/llvm/lib/Option/ArgList.cpp
index 345754dec9d7b9..3bbff6c75e78aa 100644
--- a/llvm/lib/Option/ArgList.cpp
+++ b/llvm/lib/Option/ArgList.cpp
@@ -95,13 +95,6 @@ StringRef ArgList::getLastArgValue(OptSpecifier Id, StringRef Default) const {
   return Default;
 }
 
-std::vector<std::string>
-ArgList::getAllArgValues(OptSpecifier Id0, OptSpecifier Id1) const {
-  SmallVector<const char *, 16> Values;
-  AddAllArgValues(Values, Id0, Id1);
-  return std::vector<std::string>(Values.begin(), Values.end());
-}
-
 void ArgList::addOptInFlag(ArgStringList &Output, OptSpecifier Pos,
                            OptSpecifier Neg) const {
   if (Arg *A = getLastArg(Pos, Neg))

>From ef86f24e0034473a55c1aafb4b70895bdc8acbc6 Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Tue, 22 Oct 2024 16:02:23 -0700
Subject: [PATCH 4/9] WIP to be squashed. Passes all tests.

Doc improved. Removed -iexternal from I_Group since it may be used (by MSVC)
to specify partial paths; we shouldn't diagnose uses that don't match an
existing directory. Addressed a few FIXMEs. Noted some possibly unintended
or inconsistent behavior with -ismvc. Restored implicit addition of search
patsh from EXTERNAL_INCLUDE. Identified where /external:anglebracket support
could be added.
---
 clang/docs/ReleaseNotes.rst                   | 48 ++++++++++++++-----
 clang/include/clang/Driver/Options.td         |  2 +-
 clang/include/clang/Lex/HeaderSearchOptions.h | 18 +++----
 clang/lib/Driver/Driver.cpp                   |  8 ++--
 clang/lib/Driver/ToolChains/Clang.cpp         |  7 ++-
 clang/lib/Driver/ToolChains/MSVC.cpp          | 44 +++++++++++------
 clang/lib/Lex/HeaderSearch.cpp                |  5 ++
 7 files changed, 86 insertions(+), 46 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 03c27342f31f66..462ddd65a24c19 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -418,7 +418,7 @@ Improvements to Clang's diagnostics
 - The warning for an unsupported type for a named register variable is now phrased ``unsupported type for named register variable``,
   instead of ``bad type for named register variable``. This makes it clear that the type is not supported at all, rather than being
   suboptimal in some way the error fails to mention (#GH111550).
-  
+
 - Clang now emits a ``-Wdepredcated-literal-operator`` diagnostic, even if the
   name was a reserved name, which we improperly allowed to suppress the
   diagnostic.
@@ -638,19 +638,43 @@ 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 for 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
+- 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. The MSVC behavior is that user search
-  paths are ordered before system search paths (like gcc), and search paths that
-  duplicate an earlier search path are ignored regardless of user/system kind
-  (similar to gcc, but without the preference for system search paths over
-  duplicate user search paths). These differences are observable for driver
-  invocations that pass, e.g., `-Idir1 -isystem dir2 -isystem dir1`. The gcc
-  behavior will search `dir2` before `dir1` and the MSVC behavior will search
-  `dir1` before `dir2`.
+  the same user/system kind are ignored. This ordering is not compatible with
+  the ordering that MSVC uses when paths are duplicate 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
 ^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index ec1f9199fff0de..cc555ce3a8ce62 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4563,7 +4563,7 @@ 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 : JoinedOrSeparate<["-"], "iexternal">, Group<I_Group>,
+def iexternal : JoinedOrSeparate<["-"], "iexternal">, Group<clang_i_Group>,
   Visibility<[ClangOption, CC1Option]>,
   HelpText<"Add directory to include search path with warnings suppressed">, MetaVarName<"<dir>">;
 def iexternal_after : JoinedOrSeparate<["-"], "iexternal-after">, Group<clang_i_Group>,
diff --git a/clang/include/clang/Lex/HeaderSearchOptions.h b/clang/include/clang/Lex/HeaderSearchOptions.h
index 924a41e7c6fc64..ba435843ce1cc7 100644
--- a/clang/include/clang/Lex/HeaderSearchOptions.h
+++ b/clang/include/clang/Lex/HeaderSearchOptions.h
@@ -38,20 +38,14 @@ enum IncludeDirGroup {
   /// Like Angled, but marks header maps used when building frameworks.
   IndexHeaderMap,
 
-  /// Like Angled, but marks system-like directories while retaining relative
-  /// order with user directories.
-  // FIXME: External is intended to match the semantics of the MSVC /external:I
-  // FIXME: option. MSVC handles such paths similarly to system paths, but
-  // FIXME: allows for separate specification of diagnostic level via the
-  // FIXME: /external:Wn option. That option does not affect the diagnostic
-  // FIXME: level used for system paths (those present in the INCLUDE env var).
-  // FIXME: Supporting the diagnostic level controls will likely require adding
-  // FIXME: an additional C_External enumerator to SrcMgr::CharacteristicKind.
+  /// 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 is intended to match the semantics of the MSVC
-  // /external:env option.
+  /// 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.
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index af4104e73cd9de..11edf99e3b4fc1 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1296,11 +1296,11 @@ 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 /external:I, /external: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, 
-                             options::OPT_iexternal_after)) {
+    for (auto IncludeDir : Args.getAllArgValues(options::OPT_I_Group)) {
       if (!VFS->exists(IncludeDir))
         Diag(diag::warn_missing_include_dirs) << IncludeDir;
     }
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..12c51f4ec76dc0 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1175,6 +1175,9 @@ 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;
     }
 
     // Not translated, render as usual.
@@ -1185,7 +1188,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.
 
@@ -8613,7 +8616,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);
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 410a61502629eb..4688308bdfbca7 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -644,7 +644,12 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                   "include");
   }
 
-  // FIXME: Why does the -imsvc option exist?
+  // FIXME: According to commit fd3e1ad0ce073e67c21833e56b74105ce8339ce3, the
+  // FIXME: -imsvc option is intended to behave as though the specified path
+  // FIXME: was present in %INCLUDE%, but these paths are getting added earlier
+  // FIXME: than %INCLUDE% paths with the DIA SDK paths being added in between.
+  // FIXME: Is this the intended behavior? Support for adding the DIA SDK paths
+  // FIXME: was introduced later via commit 951f362e2560fe1c9c05f487107fd9882d45d867.
   // Add %INCLUDE%-like directories from the -imsvc flag.
   for (const auto &Path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
     addSystemInclude(DriverArgs, CC1Args, Path);
@@ -665,26 +670,35 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   if (DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
-  // Add system paths from the %INCLUDE% environment variable if neither a
-  // vctoolsdir or winsysroot directory has been explicitly specified.
-  // If any paths are present in %INCLUDE%, skip adding additional system
-  // directories.
-  // FIXME: Paths specified by the INCLUDE environment variable should be
-  // FIXME: handled as system paths distinct from external paths added by
-  // FIXME: /external:I or /external:env since /external:Wn options do not
-  // FIXME: apply to these paths. These paths are currently mapped to
-  // FIXME: -iexternal-after to avoid include path ordering issues when
-  // FIXME: duplicate paths are present.
-  if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
-                             options::OPT__SLASH_winsysroot)) {
-    if (auto Val = llvm::sys::Process::GetEnv("INCLUDE")) {
+  auto AddExternalIncludesFromEnv = [&](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()) {
         addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
-        return;
+        return true;
       }
     }
+    return false;
+  };
+
+  // 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.
+  // FIXME: MSVC handles paths specified by the INCLUDE environment variable
+  // FIXME: as user paths unless an external include path specified by a
+  // FIXME: /external:I or /external:env option or the EXTERNAL_INCLUDE
+  // FIXME: environment variable is a prefix match in which case, warnings
+  // FIXME: are issued subject to the /external:Wn option. For now, all paths
+  // FIXME: specified in INCLUDE are handled as external paths which results
+  // FIXME: in them being handled as system paths.
+  if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
+                             options::OPT__SLASH_winsysroot)) {
+    bool Found = AddExternalIncludesFromEnv("INCLUDE");
+    Found |= AddExternalIncludesFromEnv("EXTERNAL_INCLUDE");
+    if (Found)
+      return;
   }
 
   // When built with access to the proper Windows APIs, try to actually find
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 8826ab449df493..b03e23f843f503 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1112,6 +1112,11 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
     if (HFI.DirInfo == SrcMgr::C_User && InUserSpecifiedSystemFramework)
       HFI.DirInfo = SrcMgr::C_System;
 
+    // FIXME: If /external:anglebrackets is enabled and the inclusion was
+    // FIXME: with angle brackets, then promote the characteristic.
+    // if (isAngled && <external:anglebrackets>)
+    //   HFI.DirInfo = SrcMgr::C_System; // Or C_External?
+
     // If the filename matches a known system header prefix, override
     // whether the file is a system header.
     for (unsigned j = SystemHeaderPrefixes.size(); j; --j) {

>From 700b98d94140c7b91de6fc2f0e0ae296813af710 Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Wed, 23 Oct 2024 10:56:20 -0700
Subject: [PATCH 5/9] WIP to be squashed. Passes all tests.

Moved expansion of env vars to driver to that the -cc1 command line always
has all paths expanded. Made -iexternal-env driver only. Still needs work
to not perform env var handling in multiple places. Modified the MSVC
test to exercise both the clang and clang-cl drivers.
---
 clang/include/clang/Driver/Options.td         |  6 +--
 clang/include/clang/Driver/ToolChain.h        |  3 ++
 clang/lib/Driver/ToolChain.cpp                | 12 +++++
 clang/lib/Driver/ToolChains/Clang.cpp         | 11 +++++
 clang/lib/Driver/ToolChains/MSVC.cpp          |  4 +-
 clang/lib/Frontend/CompilerInvocation.cpp     | 19 ++------
 clang/lib/Lex/InitHeaderSearch.cpp            |  3 +-
 clang/test/Driver/cl-include.c                | 26 ++++++----
 clang/test/Driver/cl-options.c                |  3 +-
 .../microsoft-header-search-duplicates.c      | 48 ++++++++++---------
 10 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index cc555ce3a8ce62..8799fed0e4036d 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4563,14 +4563,14 @@ 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 : JoinedOrSeparate<["-"], "iexternal">, Group<clang_i_Group>,
+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 : JoinedOrSeparate<["-"], "iexternal-after">, Group<clang_i_Group>,
+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, CC1Option]>,
+  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]>,
diff --git a/clang/include/clang/Driver/ToolChain.h b/clang/include/clang/Driver/ToolChain.h
index bcac7e17eda881..645064fcf37d8f 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/clang/include/clang/Driver/ToolChain.h
@@ -242,6 +242,9 @@ class ToolChain {
   static void addExternalAfterIncludes(const llvm::opt::ArgList &DriverArgs,
                                        llvm::opt::ArgStringList &CC1Args,
                                        ArrayRef<StringRef> Paths);
+  static void 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 = "");
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 3e83046189504d..a6bbccad014eb8 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1276,6 +1276,18 @@ void ToolChain::addExternCSystemIncludeIfExists(const ArgList &DriverArgs,
   }
 }
 
+/// Utility function to add a list of directories specified in an environment
+/// variable to the end of the external include path list for CC1.
+/*static*/ void 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);
+    addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
+  }
+}
+
 /*static*/ std::string ToolChain::concat(StringRef Path, const Twine &A,
                                          const Twine &B, const Twine &C,
                                          const Twine &D) {
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 12c51f4ec76dc0..b9f278ae2d118a 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1178,6 +1178,17 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
     } 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();
+      if (auto Val = llvm::sys::Process::GetEnv(A->getValue())) {
+        SmallVector<StringRef, 8> Dirs;
+        StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+        for (const auto &Dir : Dirs) {
+          CmdArgs.push_back("-iexternal-after");
+          CmdArgs.push_back(Args.MakeArgString(Dir));
+        }
+      }
+      continue;
     }
 
     // Not translated, render as usual.
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 4688308bdfbca7..46217476f8b1e4 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -636,6 +636,8 @@ void MSVCToolChain::AddSystemIncludeWithSubfolder(
 
 void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                               ArgStringList &CC1Args) const {
+  // FIXME: Options explicitly present in the command line should still be
+  // FIXME: processed. E.g., the /imsvc option handled below.
   if (DriverArgs.hasArg(options::OPT_nostdinc))
     return;
 
@@ -675,7 +677,7 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
       SmallVector<StringRef, 8> Dirs;
       StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
       if (!Dirs.empty()) {
-        addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
+        addExternalIncludesFromEnv(DriverArgs, CC1Args, Var);
         return true;
       }
     }
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index a2c3cfa151b617..d81f3d29d5f773 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3373,22 +3373,9 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
   }
 
   // Add the MSVC /external:env and -iexternal-after options in order.
-  for (const auto *A :
-      Args.filtered(OPT_iexternal_after, OPT_iexternal_env_EQ)) {
-    if (A->getOption().matches(OPT_iexternal_after))
-      Opts.AddPath(A->getValue(), frontend::ExternalAfter,
-                   /*IsFramework=*/false, /*IgnoreSysRoot=*/true);
-    else {
-      if (auto Val = llvm::sys::Process::GetEnv(A->getValue())) {
-        SmallVector<StringRef, 8> Dirs;
-        StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
-        for (const auto &Dir : Dirs) {
-          Opts.AddPath(Dir, frontend::ExternalAfter, /*IsFramework=*/false,
-                       /*IgnoreSysRoot=*/true);
-        }
-      }
-    }
-  }
+  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.
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index ebe2b30d80ac15..74ea87b9858e8d 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -403,6 +403,7 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
     // to ensure that #include and #include_next directives produce the same
     // results as these other compilers.
     //
+    // FIXME: The following comment isn't accurate.
     // GCC and MSVC both prune duplicate user search locations that follow a
     // previous matching user search location. Both compilers also prune user
     // search locations that are also present as system search locations
@@ -508,7 +509,7 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   RemoveDuplicates(Lang, SearchList, 0, Verbose);
   unsigned NumQuoted = SearchList.size();
 
-  // FIXME: GCC Ignore the last path for quoted inclusion when it matches the
+  // FIXME: GCC ignores the last path for quoted inclusion when it matches the
   // FIXME: first path for angled inclusion. This is observable when using
   // FIXME: #include_next.
 
diff --git a/clang/test/Driver/cl-include.c b/clang/test/Driver/cl-include.c
index a4a6cb4335f08a..e31c152eb54020 100644
--- a/clang/test/Driver/cl-include.c
+++ b/clang/test/Driver/cl-include.c
@@ -7,31 +7,37 @@
 // 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: "-iexternal-after" "/my/system/inc"
+// STDINC: "-iexternal-after" "/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: "-iexternal-after" "/my/system/inc"
+// NOSTDINC-NOT: "-iexternal-after" "/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, -imsvc dirs, or /external: flags.
-// RUN: env INCLUDE=/my/system/inc env FOO=/my/other/inc2 %clang_cl /X -imsvc /my/other/inc /external:env:FOO -### -- %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: "-external-after" "/my/system/inc"
-// SLASHX: "-iexternal-env=FOO"
+// SLASHX-NOT: "-external-after" "/my/system/inc2"
+// SLASHX: "-iexternal-after" "/my/other/inc2"
 // SLASHX: "-internal-isystem" "{{.*lib.*clang.*include}}"
 // SLASHX: "-internal-isystem" "/my/other/inc"
 
-// /winsysroot does not suppress -imsvc dirs or /external: flags.
-// RUN: env FOO=/my/other/inc2 %clang_cl /winsysroot /foo -imsvc /my/other/inc /external:env:FOO -### -- %s 2>&1 | FileCheck %s --check-prefix=SYSROOT
+// /winsysroot suppresses %EXTERNAL_INCLUDE% but not -imsvc dirs or /external: flags.
+// RUN: 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: "-iexternal-env=FOO"
+// SYSROOT-NOT: "/my/system/inc"
+// SYSROOT: "-iexternal-after" "/my/other/inc2"
 // SYSROOT: "-internal-isystem" "/my/other/inc"
 // 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: "-iexternal-env=FOO"
-// EXTERNAL_ENV: "-iexternal-env=BAR"
+// EXTERNAL_ENV: "-iexternal-after" "/dir1"
+// EXTERNAL_ENV: "-iexternal-after" "/dir2"
+// EXTERNAL_ENV: "-iexternal-after" "/dir3"
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index ec194cadae3d8a..c3885c2db45778 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -43,7 +43,8 @@
 // EXTERNAL_I: "-iexternal" "path"
 
 // RUN: env EXTPATH="path1;path2" %clang_cl /external:env:EXTPATH -### -- %s 2>&1 | FileCheck -check-prefix=EXTERNAL_ENV %s
-// EXTERNAL_ENV: "-iexternal-env=EXTPATH"
+// EXTERNAL_ENV: "-iexternal-after" "path1"
+// EXTERNAL_ENV: "-iexternal-after" "path2"
 
 // RUN: %clang_cl /fp:fast /fp:except -### -- %s 2>&1 | FileCheck -check-prefix=fpexcept %s
 // fpexcept-NOT: -funsafe-math-optimizations
diff --git a/clang/test/Driver/microsoft-header-search-duplicates.c b/clang/test/Driver/microsoft-header-search-duplicates.c
index 05157270590d36..57f974ff2fa971 100644
--- a/clang/test/Driver/microsoft-header-search-duplicates.c
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -8,29 +8,31 @@
 // Test the clang driver with a Windows target that implicitly enables the
 // -fms-compatibility option. The -nostdinc option is used to suppress default
 // search paths to ease testing.
-// XUN: env INCLUDE="%t/include/p;%t/include/o" \
-// XUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
-// XUN  %clang \
-// XUN:     -target x86_64-pc-windows \
-// XUN:     -v -fsyntax-only \
-// XUN:     -nostdinc \
-// XUN:     -isystem %t/include/s \
-// XUN:     -isystem %t/include/w \
-// XUN:     -I%t/include/v \
-// XUN:     -isystem %t/include/x \
-// XUN:     -I%t/include/z \
-// XUN:     -isystem %t/include/y \
-// XUN:     -I%t/include/r \
-// XUN:     -external:env:EXTRA_INCLUDE \
-// XUN:     -I%t/include/t \
-// XUN:     -isystem %t/include/z \
-// XUN:     -I%t/include/o \
-// XUN:     -I%t/include/x \
-// XUN:     -isystem %t/include/y \
-// XUN:     -I%t/include/v \
-// XUN:     -isystem %t/include/w \
-// XUN:     -I%t/include/u \
-// XUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
+// RUN: env INCLUDE="%t/include/p;%t/include/o" \
+// RUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
+// RUN: %clang \
+// RUN:     -target x86_64-pc-windows \
+// RUN:     -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -iexternal %t/include/s \
+// RUN:     -iexternal %t/include/w \
+// RUN:     -I%t/include/v \
+// RUN:     -iexternal %t/include/x \
+// RUN:     -I%t/include/z \
+// RUN:     -iexternal %t/include/y \
+// RUN:     -I%t/include/r \
+// RUN:     -iexternal-env=EXTRA_INCLUDE \
+// RUN:     -I%t/include/t \
+// RUN:     -iexternal %t/include/z \
+// RUN:     -I%t/include/o \
+// RUN:     -I%t/include/x \
+// RUN:     -iexternal %t/include/y \
+// RUN:     -I%t/include/v \
+// RUN:     -iexternal %t/include/w \
+// RUN:     -I%t/include/u \
+// RUN:     -iexternal-env=INCLUDE \
+// RUN:     -iexternal-env=EXTERNAL_INCLUDE \
+// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
 
 // Test the clang-cl driver with a Windows target that implicitly enables the
 // -fms-compatibility option. The -nobuiltininc option is uesd to suppress the

>From 36f7af48cd4d6459eb8b9eaf00b562945746deab Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Wed, 23 Oct 2024 21:39:18 -0700
Subject: [PATCH 6/9] WIP to be squashed. Passes all tests.

Updated RemoveDuplicates() to emit the "as it is a non-system directory that
duplicates a system directory" message in the MSVC specific case in which an
earlier external path duplicates a later user path. This is also a first step
towards more formalizing the header search partition management when removing
duplicate paths.
---
 clang/lib/Lex/InitHeaderSearch.cpp            | 138 ++++++++++--------
 .../microsoft-header-search-duplicates.c      |  24 +--
 2 files changed, 89 insertions(+), 73 deletions(-)

diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index 74ea87b9858e8d..a2a638cb44eaea 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -369,16 +369,17 @@ void InitHeaderSearch::AddDefaultIncludePaths(
 
 /// If there are duplicate directory entries in the specified search list,
 /// identify and remove the ones to be ignored and issue a diagnostic.
-/// Returns the number of non-system search paths removed.
+/// Returns the number of lookback search paths removed.
 static unsigned RemoveDuplicates(const LangOptions &Lang,
                                  std::vector<DirectoryLookupInfo> &SearchList,
-                                 unsigned First, bool Verbose,
+                                 unsigned Lookback, unsigned First,
+                                 bool Verbose,
                                  bool RemovePreviousAllowed = true) {
   llvm::SmallPtrSet<const DirectoryEntry *, 8> SeenDirs;
   llvm::SmallPtrSet<const DirectoryEntry *, 8> SeenFrameworkDirs;
   llvm::SmallPtrSet<const HeaderMap *, 8> SeenHeaderMaps;
-  unsigned NonSystemRemoved = 0;
-  for (unsigned i = First; i != SearchList.size(); ++i) {
+  unsigned NumLookbackDirsRemoved = 0;
+  for (unsigned i = Lookback; i != SearchList.size(); ++i) {
     unsigned DirToRemove = i;
     bool NonSystemDirRemoved = false;
 
@@ -411,47 +412,51 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
     // respect to pruning system search locations that duplicate a previous
     // system search location; GCC preserves the first such occurence while
     // MSVC preserves the last one.
-    if (RemovePreviousAllowed &&
-        CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
-      // Find the matching search entry.
-      unsigned FirstDir;
-      for (FirstDir = First; FirstDir < i; ++FirstDir) {
-        const DirectoryLookup &SearchEntry = SearchList[FirstDir].Lookup;
-
-        // Different lookup types are not considered duplicate entries.
-        if (SearchEntry.getLookupType() != CurEntry.getLookupType())
-          continue;
-
-        bool isSame;
-        if (CurEntry.isNormalDir())
-          isSame = SearchEntry.getDir() == CurEntry.getDir();
-        else if (CurEntry.isFramework())
-          isSame = SearchEntry.getFrameworkDir() == CurEntry.getFrameworkDir();
-        else {
-          assert(CurEntry.isHeaderMap() && "Not a headermap or normal dir?");
-          isSame = SearchEntry.getHeaderMap() == CurEntry.getHeaderMap();
-        }
-
-        if (isSame)
-          break;
+
+    // Find the matching search entry.
+    unsigned FirstDir;
+    for (FirstDir = Lookback; FirstDir < i; ++FirstDir) {
+      const DirectoryLookup &SearchEntry = SearchList[FirstDir].Lookup;
+
+      // Different lookup types are not considered duplicate entries.
+      if (SearchEntry.getLookupType() != CurEntry.getLookupType())
+        continue;
+
+      bool isSame;
+      if (CurEntry.isNormalDir())
+        isSame = SearchEntry.getDir() == CurEntry.getDir();
+      else if (CurEntry.isFramework())
+        isSame = SearchEntry.getFrameworkDir() == CurEntry.getFrameworkDir();
+      else {
+        assert(CurEntry.isHeaderMap() && "Not a headermap or normal dir?");
+        isSame = SearchEntry.getHeaderMap() == CurEntry.getHeaderMap();
       }
-      assert(FirstDir < i && "Expected duplicate search location not found");
 
-      if (Lang.MSVCCompat) {
-        // In Microsoft compatibility mode, a later system search location entry
-        // suppresses a previous user or system search location.
+      if (isSame)
+        break;
+    }
+    assert(FirstDir < i && "Expected duplicate search location not found");
+
+    if (Lang.MSVCCompat) {
+      // In Microsoft compatibility mode, an external search path suppresses
+      // a user search path regardless of order.
+      if (RemovePreviousAllowed &&
+          CurEntry.getDirCharacteristic() != SrcMgr::C_User) {
         DirToRemove = FirstDir;
-        if (SearchList[FirstDir].Lookup.getDirCharacteristic() ==
-            SrcMgr::C_User)
-          NonSystemDirRemoved = true;
-      } else {
-        // In GCC compatibility mode, a later system search location entry
-        // suppresses a previous user search location.
-        if (SearchList[FirstDir].Lookup.getDirCharacteristic() ==
-            SrcMgr::C_User) {
-          DirToRemove = FirstDir;
-          NonSystemDirRemoved = true;
-        }
+        NonSystemDirRemoved =
+          SearchList[FirstDir].Lookup.getDirCharacteristic() == SrcMgr::C_User;
+      } else if (CurEntry.getDirCharacteristic() == SrcMgr::C_User) {
+        NonSystemDirRemoved =
+          SearchList[FirstDir].Lookup.getDirCharacteristic() != SrcMgr::C_User;
+      }
+    } else {
+      // In GCC compatibility mode, a later system search location entry
+      // suppresses a previous user search location.
+      if (RemovePreviousAllowed &&
+          CurEntry.getDirCharacteristic() != SrcMgr::C_User &&
+          SearchList[FirstDir].Lookup.getDirCharacteristic() == SrcMgr::C_User) {
+        DirToRemove = FirstDir;
+        NonSystemDirRemoved = true;
       }
     }
 
@@ -462,15 +467,20 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
         llvm::errs() << "  as it is a non-system directory that duplicates "
                      << "a system directory\n";
     }
-    if (NonSystemDirRemoved)
-      ++NonSystemRemoved;
 
     // This is reached if the current entry is a duplicate.  Remove the
     // DirToRemove (usually the current dir).
     SearchList.erase(SearchList.begin()+DirToRemove);
     --i;
+
+    // If the removed directory is from the lookback area, update the pivot
+    // index.
+    if (DirToRemove < First) {
+      ++NumLookbackDirsRemoved;
+      --First;
+    }
   }
-  return NonSystemRemoved;
+  return NumLookbackDirsRemoved;
 }
 
 /// Extract DirectoryLookups from DirectoryLookupInfos.
@@ -504,34 +514,39 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   for (auto &Include : IncludePath)
     if (Include.Group == Quoted)
       SearchList.push_back(Include);
-
   // Remove duplicate search paths within the quoted inclusion list.
-  RemoveDuplicates(Lang, SearchList, 0, Verbose);
-  unsigned NumQuoted = SearchList.size();
+  RemoveDuplicates(Lang, SearchList, 0, 0, Verbose);
+  unsigned EndQuoted = SearchList.size();
 
   // FIXME: GCC ignores the last path for quoted inclusion when it matches the
   // FIXME: first path for angled inclusion. This is observable when using
   // FIXME: #include_next.
 
-  // Add search paths for angled inclusion next.
+  // Add search paths for angled inclusion next. Note that user paths and
+  // external paths may be interleaved; though external paths are treated like
+  // system paths, they are not reordered to the end of the search list.
   for (auto &Include : IncludePath)
     if (Include.Group == Angled || Include.Group == IndexHeaderMap ||
         Include.Group == External)
       SearchList.push_back(Include);
-
   // Remove duplicate search paths within the angled inclusion list.
   // This may leave paths duplicated across the quoted and angled inclusion
-  // sections.
-  RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
-  unsigned NumAngled = SearchList.size();
+  // sections. Note that user paths that duplicate one of these external
+  // paths may be removed regardless of order (in Microsoft compatibility
+  // mode).
+  RemoveDuplicates(Lang, SearchList, EndQuoted, EndQuoted, Verbose);
+  unsigned EndAngled = SearchList.size();
 
-  // Add search paths for external-after inclusion next and remove duplicates
-  // within them.
+  // Add search paths for external-after inclusion next.
   for (auto &Include : IncludePath)
     if (Include.Group == ExternalAfter)
       SearchList.push_back(Include);
-  RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose,
+  // Remove duplicate search paths within the angled inclusion list, but
+  // always remove the later duplicate in this case; ExternalAfter paths
+  // never suppress a External path.
+  RemoveDuplicates(Lang, SearchList, EndQuoted, EndAngled, Verbose,
                    /*RemovePreviousAllowed*/ false);
+  EndAngled = SearchList.size();
 
   // Add search paths for language dependent system paths next.
   for (auto &Include : IncludePath)
@@ -550,12 +565,13 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   // Remove duplicate search paths across both the angled inclusion list and
   // the system search paths. This duplicate removal is necessary to ensure
   // that header lookup in #include_next directives doesn't resolve to the
-  // same file.
+  // same file. This may result in earlier user and external paths being
+  // removed, and thus requires updating the EndAngled index.
   unsigned NonSystemRemoved =
-      RemoveDuplicates(Lang, SearchList, NumQuoted, Verbose);
-  NumAngled -= NonSystemRemoved;
+      RemoveDuplicates(Lang, SearchList, EndQuoted, EndAngled, Verbose);
+  EndAngled -= NonSystemRemoved;
 
-  Headers.SetSearchPaths(extractLookups(SearchList), NumQuoted, NumAngled,
+  Headers.SetSearchPaths(extractLookups(SearchList), EndQuoted, EndAngled,
                          mapToUserEntries(SearchList));
 
   Headers.SetSystemHeaderPrefixes(SystemHeaderPrefixes);
@@ -564,7 +580,7 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   if (Verbose) {
     llvm::errs() << "#include \"...\" search starts here:\n";
     for (unsigned i = 0, e = SearchList.size(); i != e; ++i) {
-      if (i == NumQuoted)
+      if (i == EndQuoted)
         llvm::errs() << "#include <...> search starts here:\n";
       StringRef Name = SearchList[i].Lookup.getName();
       const char *Suffix;
diff --git a/clang/test/Driver/microsoft-header-search-duplicates.c b/clang/test/Driver/microsoft-header-search-duplicates.c
index 57f974ff2fa971..8d4a34c83e2d3d 100644
--- a/clang/test/Driver/microsoft-header-search-duplicates.c
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -91,18 +91,18 @@
 // 3) Paths specified by the %INCLUDE% environment variable are processed in
 //    order. Paths that duplicate any other path are ignored.
 
-// XHECK:      ignoring duplicate directory "[[PWD]]/include/v"
-// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
-// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/z"
-// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/t"
-// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/r"
-// XHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
-// XHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
-// CHECK:      #include <...> search starts here:
+// CHECK:      ignoring duplicate directory "[[PWD]]/include/z"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/v"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/t"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/r"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/o"
+// CHECK-NEXT: #include "..." search starts here:
+// CHECK-NEXT: #include <...> search starts here:
 // CHECK-NEXT: [[PWD]]/include/s
 // CHECK-NEXT: [[PWD]]/include/v
 // CHECK-NEXT: [[PWD]]/include/x

>From d1b8e86194c57c877ec04bea14cba5ffbdc35d75 Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Thu, 24 Oct 2024 14:06:45 -0700
Subject: [PATCH 7/9] WIP to be squashed. Test updates.

Refactor microsoft-header-search-duplicates.c to test a smaller set of
scenarios at a time. Documented the test.
---
 .../microsoft-header-search-duplicates.c      | 555 +++++++++---------
 1 file changed, 289 insertions(+), 266 deletions(-)

diff --git a/clang/test/Driver/microsoft-header-search-duplicates.c b/clang/test/Driver/microsoft-header-search-duplicates.c
index 8d4a34c83e2d3d..5a30e3be21ef36 100644
--- a/clang/test/Driver/microsoft-header-search-duplicates.c
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -1,340 +1,363 @@
 // Test that pruning of header search paths emulates MSVC behavior when in
-// Microsoft compatibility mode.
-// See header-search-duplicates.c for GCC compatible behavior.
+// Microsoft compatibility mode. See header-search-duplicates.c for GCC
+// compatible behavior.
+
+// This test is intended to be usable to validate MSVC behavior using a .bat
+// test driver similar to the following. A failure to compile successfully
+// would indicate a problem with the test or, perhaps, a behavioral difference
+// across MSVC versions.
+//   @echo on
+//   setlocal
+//   rd /s/q test-msvc
+//   split-file microsoft-header-search-duplicates.c test-msvc
+//   pushd test-msvc
+//   REM Validate test 1:
+//   set INCLUDE=...
+//   set EXTERNAL_INCLUDE=...
+//   set EXTRA_INCLUDE=...
+//   cl.exe /c /showIncludes ... test1.c
+//   popd
+
+// This test exercises both the clang and clang-cl drivers using a target that
+// implicitly enables the '-fms-compatibility' option. The '-nobuiltininc',
+// '-nostdinc', and '/X ('-nostdlibinc') options are used to suppress implicit
+// header search paths to ease testing.
+
+// Header search paths are processed as follows:
+// 1) Paths specified by the '/I' and '/external:I' options are processed in
+//    order.
+//    1.1) Paths specified by '/I' that duplicate a path specified by
+//         '/external:I' are ignored regardless of the option order.
+//    1.2) Paths specified by '/I' that duplicate a prior '/I' option are
+//         ignored.
+//    1.3) Paths specified by '/external:I' that duplicate a later
+//         '/external:I' option are ignored.
+// 2) Paths specified by the '/external:env' options are processed in order.
+//    Paths that duplicate a path from step 1, a prior '/external:env' option,
+//    or a prior path from the current '/external:env' option are ignored.
+// 3) Paths specified by the 'INCLUDE' environment variable are processed in
+//    order. Paths that duplicate a path from step 1, step 2, or an earlier
+//    path in the 'INCLUDE' environment variable are ignored.
+// 4) Paths specified by the 'EXTERNALINCLUDE' environment variable are
+//    processed in order. Paths that duplicate a path from step 1, step 2,
+//    step 3, or an earlier path in the 'EXTERNAL_INCLUDE' environment
+//    variable are ignored.
 
 // RUN: rm -rf %t
 // RUN: split-file %s %t
 
-// Test the clang driver with a Windows target that implicitly enables the
-// -fms-compatibility option. The -nostdinc option is used to suppress default
-// search paths to ease testing.
-// RUN: env INCLUDE="%t/include/p;%t/include/o" \
-// RUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
+
+// Test 1: Validate ordering and duplicate elimination for /I.
+//
 // RUN: %clang \
-// RUN:     -target x86_64-pc-windows \
-// RUN:     -v -fsyntax-only \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
 // RUN:     -nostdinc \
-// RUN:     -iexternal %t/include/s \
-// RUN:     -iexternal %t/include/w \
-// RUN:     -I%t/include/v \
-// RUN:     -iexternal %t/include/x \
-// RUN:     -I%t/include/z \
-// RUN:     -iexternal %t/include/y \
-// RUN:     -I%t/include/r \
-// RUN:     -iexternal-env=EXTRA_INCLUDE \
-// RUN:     -I%t/include/t \
-// RUN:     -iexternal %t/include/z \
-// RUN:     -I%t/include/o \
-// RUN:     -I%t/include/x \
-// RUN:     -iexternal %t/include/y \
-// RUN:     -I%t/include/v \
-// RUN:     -iexternal %t/include/w \
-// RUN:     -I%t/include/u \
-// RUN:     -iexternal-env=INCLUDE \
-// RUN:     -iexternal-env=EXTERNAL_INCLUDE \
-// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
-
-// Test the clang-cl driver with a Windows target that implicitly enables the
-// -fms-compatibility option. The -nobuiltininc option is uesd to suppress the
-// Clang resource directory. The -nostdlibinc option is used to suppress search
-// paths for the Windows SDK, CRT, MFC, ATL, etc...
-// RUN: env INCLUDE="%t/include/p;%t/include/o" \
-// RUN: env EXTRA_INCLUDE="%t/include/t;%t/include/q;%t/include/r" \
+// RUN:     -I%t/test1/include/y \
+// RUN:     -I%t/test1/include/z \
+// RUN:     -I%t/test1/include/y \
+// RUN:     %t/test1/t.c 2>&1 | FileCheck -DPWD=%t %t/test1/t.c
 // RUN: %clang_cl \
-// RUN:     -target x86_64-pc-windows \
-// RUN:     -v -fsyntax-only \
-// RUN:     -nobuiltininc \
-// RUN:     -nostdlibinc \
-// RUN:     /external:I %t/include/s \
-// RUN:     /external:I %t/include/w \
-// RUN:     /I%t/include/v \
-// RUN:     /external:I %t/include/x \
-// RUN:     /I%t/include/z \
-// RUN:     /external:I %t/include/y \
-// RUN:     /I%t/include/r \
-// RUN:     /external:env:EXTRA_INCLUDE \
-// RUN:     /I%t/include/t \
-// RUN:     /external:I %t/include/z \
-// RUN:     /I%t/include/o \
-// RUN:     /I%t/include/x \
-// RUN:     /external:I %t/include/y \
-// RUN:     /I%t/include/v \
-// RUN:     /external:I %t/include/w \
-// RUN:     /I%t/include/u \
-// RUN:     %t/test.c 2>&1 | FileCheck -DPWD=%t %t/test.c
-
-#--- test.c
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nobuiltininc /X \
+// RUN:     /I%t/test1/include/y \
+// RUN:     /I%t/test1/include/z \
+// RUN:     /I%t/test1/include/y \
+// RUN:     %t/test1/t.c 2>&1 | FileCheck -DPWD=%t %t/test1/t.c
+
+#--- test1/t.c
 #include <a.h>
 #include <b.h>
-#include <c.h>
-#include <d.h>
-#include <e.h>
-#include <f.h>
-#include <g.h>
-#include <h.h>
-#include <i.h>
-#include <j.h>
-#include <k.h>
-#include <l.h>
 
-// Header search paths are processed as follows:
-// 1) Paths specified by the /I and /external:I options are processed in order.
-//    1.1) Paths specified by /I that duplicate a path specified by /external:I
-//         are ignored regardless of the option order.
-//    1.2) Paths specified by /I that duplicate a prior /I option are ignored.
-//    1.3) Paths specified by /external:I that duplicate a later /external:I
-//         option are ignored.
-// 2) Paths specified by the /external:env options are processed in order.
-//    Paths that duplicate a path from a /I option, an external:I option, a
-//    prior /external:env option, or a prior path from the current /external:env
-//    option are ignored.
-// 3) Paths specified by the %INCLUDE% environment variable are processed in
-//    order. Paths that duplicate any other path are ignored.
-
-// CHECK:      ignoring duplicate directory "[[PWD]]/include/z"
-// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/x"
-// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/y"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/v"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/w"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/t"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/r"
-// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/include/o"
+// CHECK:      ignoring duplicate directory "[[PWD]]/test1/include/y"
 // CHECK-NEXT: #include "..." search starts here:
 // CHECK-NEXT: #include <...> search starts here:
-// CHECK-NEXT: [[PWD]]/include/s
-// CHECK-NEXT: [[PWD]]/include/v
-// CHECK-NEXT: [[PWD]]/include/x
-// CHECK-NEXT: [[PWD]]/include/r
-// CHECK-NEXT: [[PWD]]/include/t
-// CHECK-NEXT: [[PWD]]/include/z
-// CHECK-NEXT: [[PWD]]/include/o
-// CHECK-NEXT: [[PWD]]/include/y
-// CHECK-NEXT: [[PWD]]/include/w
-// CHECK-NEXT: [[PWD]]/include/u
-// CHECK-NEXT: [[PWD]]/include/q
-// CHECK-NEXT: [[PWD]]/include/p
+// CHECK-NEXT: [[PWD]]/test1/include/y
+// CHECK-NEXT: [[PWD]]/test1/include/z
 // CHECK-NEXT: End of search list.
 
-#--- include/s/a.h
-
-#--- include/v/a.h
-#error 'include/v/a.h' should not have been included!
-
-#--- include/v/b.h
-
-#--- include/x/a.h
-#error 'include/x/a.h' should not have been included!
-
-#--- include/x/b.h
-#error 'include/x/b.h' should not have been included!
-
-#--- include/x/c.h
+#--- test1/include/y/a.h
 
-#--- include/r/a.h
-#error 'include/r/a.h' should not have been included!
+#--- test1/include/z/a.h
+#error 'test1/include/z/a.h' should not have been included!
 
-#--- include/r/b.h
-#error 'include/r/b.h' should not have been included!
+#--- test1/include/z/b.h
 
-#--- include/r/c.h
-#error 'include/r/c.h' should not have been included!
 
-#--- include/r/d.h
-
-#--- include/t/a.h
-#error 'include/t/a.h' should not have been included!
-
-#--- include/t/b.h
-#error 'include/t/b.h' should not have been included!
-
-#--- include/t/c.h
-#error 'include/t/c.h' should not have been included!
-
-#--- include/t/d.h
-#error 'include/t/cdh' should not have been included!
-
-#--- include/t/e.h
-
-#--- include/z/a.h
-#error 'include/z/a.h' should not have been included!
-
-#--- include/z/b.h
-#error 'include/z/b.h' should not have been included!
-
-#--- include/z/c.h
-#error 'include/z/c.h' should not have been included!
-
-#--- include/z/d.h
-#error 'include/z/d.h' should not have been included!
-
-#--- include/z/e.h
-#error 'include/z/e.h' should not have been included!
-
-#--- include/z/f.h
-
-#--- include/o/a.h
-#error 'include/o/a.h' should not have been included!
-
-#--- include/o/b.h
-#error 'include/o/b.h' should not have been included!
+// Test 2: Validate ordering and duplicate elimination for /external:I.
+//
+// RUN: %clang \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -iexternal %t/test2/include/z \
+// RUN:     -iexternal %t/test2/include/y \
+// RUN:     -iexternal %t/test2/include/z \
+// RUN:     %t/test2/t.c 2>&1 | FileCheck -DPWD=%t %t/test2/t.c
+// RUN: %clang_cl \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nobuiltininc /X \
+// RUN:     /external:I %t/test2/include/z \
+// RUN:     /external:I %t/test2/include/y \
+// RUN:     /external:I %t/test2/include/z \
+// RUN:     %t/test2/t.c 2>&1 | FileCheck -DPWD=%t %t/test2/t.c
+
+#--- test2/t.c
+#include <a.h>
+#include <b.h>
 
-#--- include/o/c.h
-#error 'include/o/c.h' should not have been included!
+// CHECK:      ignoring duplicate directory "[[PWD]]/test2/include/z"
+// CHECK-NEXT: #include "..." search starts here:
+// CHECK-NEXT: #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/test2/include/y
+// CHECK-NEXT: [[PWD]]/test2/include/z
+// CHECK-NEXT: End of search list.
 
-#--- include/o/d.h
-#error 'include/o/d.h' should not have been included!
+#--- test2/include/y/a.h
 
-#--- include/o/e.h
-#error 'include/o/e.h' should not have been included!
+#--- test2/include/z/a.h
+#error 'test2/include/z/a.h' should not have been included!
 
-#--- include/o/f.h
-#error 'include/o/f.h' should not have been included!
+#--- test2/include/z/b.h
 
-#--- include/o/g.h
 
-#--- include/y/a.h
-#error 'include/y/a.h' should not have been included!
+// Test 3: Validate ordering and duplicate elimination for /I vs /external:I.
+//
+// RUN: %clang \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -iexternal %t/test3/include/w \
+// RUN:     -I%t/test3/include/z \
+// RUN:     -I%t/test3/include/x \
+// RUN:     -I%t/test3/include/w \
+// RUN:     -iexternal %t/test3/include/y \
+// RUN:     -iexternal %t/test3/include/z \
+// RUN:     %t/test3/t.c 2>&1 | FileCheck -DPWD=%t %t/test3/t.c
+// RUN: %clang_cl \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nobuiltininc /X \
+// RUN:     /external:I %t/test3/include/w \
+// RUN:     /I%t/test3/include/z \
+// RUN:     /I%t/test3/include/x \
+// RUN:     /I%t/test3/include/w \
+// RUN:     /external:I %t/test3/include/y \
+// RUN:     /external:I %t/test3/include/z \
+// RUN:     %t/test3/t.c 2>&1 | FileCheck -DPWD=%t %t/test3/t.c
+
+#--- test3/t.c
+#include <a.h>
+#include <b.h>
+#include <c.h>
+#include <d.h>
 
-#--- include/y/b.h
-#error 'include/y/b.h' should not have been included!
+// CHECK:      ignoring duplicate directory "[[PWD]]/test3/include/w"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test3/include/z"
+// CHECK-NEXT:  as it is a non-system directory that duplicates a system directory
+// CHECK-NEXT: #include "..." search starts here:
+// CHECK-NEXT: #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/test3/include/w
+// CHECK-NEXT: [[PWD]]/test3/include/x
+// CHECK-NEXT: [[PWD]]/test3/include/y
+// CHECK-NEXT: [[PWD]]/test3/include/z
+// CHECK-NEXT: End of search list.
 
-#--- include/y/c.h
-#error 'include/y/c.h' should not have been included!
+#--- test3/include/w/a.h
 
-#--- include/y/d.h
-#error 'include/y/d.h' should not have been included!
+#--- test3/include/x/a.h
+#error 'test3/include/x/a.h' should not have been included!
 
-#--- include/y/e.h
-#error 'include/y/e.h' should not have been included!
+#--- test3/include/x/b.h
 
-#--- include/y/f.h
-#error 'include/y/f.h' should not have been included!
+#--- test3/include/y/a.h
+#error 'test3/include/y/a.h' should not have been included!
 
-#--- include/y/g.h
-#error 'include/y/g.h' should not have been included!
+#--- test3/include/y/b.h
+#error 'test3/include/y/b.h' should not have been included!
 
-#--- include/y/h.h
+#--- test3/include/y/c.h
 
-#--- include/w/a.h
-#error 'include/w/a.h' should not have been included!
+#--- test3/include/z/a.h
+#error 'test3/include/z/a.h' should not have been included!
 
-#--- include/w/b.h
-#error 'include/w/b.h' should not have been included!
+#--- test3/include/z/b.h
+#error 'test3/include/z/b.h' should not have been included!
 
-#--- include/w/c.h
-#error 'include/w/c.h' should not have been included!
+#--- test3/include/z/c.h
+#error 'test3/include/z/c.h' should not have been included!
 
-#--- include/w/d.h
-#error 'include/w/d.h' should not have been included!
+#--- test3/include/z/d.h
 
-#--- include/w/e.h
-#error 'include/w/e.h' should not have been included!
 
-#--- include/w/f.h
-#error 'include/w/f.h' should not have been included!
+// Test 4: Validate ordering and duplicate elimination for /external:env.
+//
+// RUN: env EXTRA_INCLUDE1="%t/test4/include/y" \
+// RUN: env EXTRA_INCLUDE2="%t/test4/include/z;%t/test4/include/y;%t/test4/include/x;%t/test4/include/w" \
+// RUN: %clang \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -I%t/test4/include/w \
+// RUN:     -iexternal %t/test4/include/x \
+// RUN:     -iexternal-env=EXTRA_INCLUDE1 \
+// RUN:     -iexternal-env=EXTRA_INCLUDE2 \
+// RUN:     %t/test4/t.c 2>&1 | FileCheck -DPWD=%t %t/test4/t.c
+// RUN: env EXTRA_INCLUDE1="%t/test4/include/y" \
+// RUN: env EXTRA_INCLUDE2="%t/test4/include/z;%t/test4/include/y;%t/test4/include/x;%t/test4/include/w" \
+// RUN: %clang_cl \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nobuiltininc /X \
+// RUN:     /I%t/test4/include/w \
+// RUN:     /external:I %t/test4/include/x \
+// RUN:     /external:env:EXTRA_INCLUDE1 \
+// RUN:     /external:env:EXTRA_INCLUDE2 \
+// RUN:     %t/test4/t.c 2>&1 | FileCheck -DPWD=%t %t/test4/t.c
+
+#--- test4/t.c
+#include <a.h>
+#include <b.h>
+#include <c.h>
+#include <d.h>
 
-#--- include/w/g.h
-#error 'include/w/g.h' should not have been included!
+// CHECK:      ignoring duplicate directory "[[PWD]]/test4/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test4/include/x"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test4/include/w"
+// CHECK-NEXT: #include "..." search starts here:
+// CHECK-NEXT: #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/test4/include/w
+// CHECK-NEXT: [[PWD]]/test4/include/x
+// CHECK-NEXT: [[PWD]]/test4/include/y
+// CHECK-NEXT: [[PWD]]/test4/include/z
+// CHECK-NEXT: End of search list.
 
-#--- include/w/h.h
-#error 'include/w/h.h' should not have been included!
+#--- test4/include/w/a.h
 
-#--- include/w/i.h
+#--- test4/include/x/a.h
+#error 'test4/include/x/a.h' should not have been included!
 
-#--- include/u/a.h
-#error 'include/u/a.h' should not have been included!
+#--- test4/include/x/b.h
 
-#--- include/u/b.h
-#error 'include/u/b.h' should not have been included!
+#--- test4/include/y/a.h
+#error 'test4/include/y/a.h' should not have been included!
 
-#--- include/u/c.h
-#error 'include/u/c.h' should not have been included!
+#--- test4/include/y/b.h
+#error 'test4/include/y/b.h' should not have been included!
 
-#--- include/u/d.h
-#error 'include/u/d.h' should not have been included!
+#--- test4/include/y/c.h
 
-#--- include/u/e.h
-#error 'include/u/e.h' should not have been included!
+#--- test4/include/z/a.h
+#error 'test4/include/z/a.h' should not have been included!
 
-#--- include/u/f.h
-#error 'include/u/f.h' should not have been included!
+#--- test4/include/z/b.h
+#error 'test4/include/z/b.h' should not have been included!
 
-#--- include/u/g.h
-#error 'include/u/g.h' should not have been included!
+#--- test4/include/z/c.h
+#error 'test4/include/z/c.h' should not have been included!
 
-#--- include/u/h.h
-#error 'include/u/h.h' should not have been included!
+#--- test4/include/z/d.h
 
-#--- include/u/i.h
-#error 'include/u/i.h' should not have been included!
 
-#--- include/u/j.h
+// Test 5: Validate ordering and duplicate elimination for the INCLUDE and
+// EXTERNAL_INCLUDE environment variables.
+//
+// RUN: env EXTRA_INCLUDE="%t/test5/include/w" \
+// RUN: env INCLUDE="%t/test5/include/x;%t/test5/include/y;%t/test5/include/w;%t/test5/include/v;%t/test5/include/u" \
+// RUN: env EXTERNAL_INCLUDE="%t/test5/include/z;%t/test5/include/y;%t/test5/include/w;%t/test5/include/v;%t/test5/include/u" \
+// RUN: %clang \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nostdinc \
+// RUN:     -I%t/test5/include/u \
+// RUN:     -iexternal %t/test5/include/v \
+// RUN:     -iexternal-env=EXTRA_INCLUDE \
+// RUN:     -iexternal-env=INCLUDE \
+// RUN:     -iexternal-env=EXTERNAL_INCLUDE \
+// RUN:     %t/test5/t.c 2>&1 | FileCheck -DPWD=%t %t/test5/t.c
+// RUN: env EXTRA_INCLUDE="%t/test5/include/w" \
+// RUN: env INCLUDE="%t/test5/include/x;%t/test5/include/y;%t/test5/include/w;%t/test5/include/v;%t/test5/include/u" \
+// RUN: env EXTERNAL_INCLUDE="%t/test5/include/z;%t/test5/include/y;%t/test5/include/w;%t/test5/include/v;%t/test5/include/u" \
+// RUN: %clang_cl \
+// RUN:     -target x86_64-pc-windows -v -fsyntax-only \
+// RUN:     -nobuiltininc \
+// RUN:     /I%t/test5/include/u \
+// RUN:     /external:I %t/test5/include/v \
+// RUN:     /external:env:EXTRA_INCLUDE \
+// RUN:     %t/test5/t.c 2>&1 | FileCheck -DPWD=%t %t/test5/t.c
 
-#--- include/q/a.h
-#error 'include/q/a.h' should not have been included!
+#--- test5/t.c
+#include <a.h>
+#include <b.h>
+#include <c.h>
+#include <d.h>
+#include <e.h>
+#include <f.h>
 
-#--- include/q/b.h
-#error 'include/q/b.h' should not have been included!
+// CHECK:      ignoring duplicate directory "[[PWD]]/test5/include/w"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test5/include/v"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test5/include/u"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test5/include/y"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test5/include/w"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test5/include/v"
+// CHECK-NEXT: ignoring duplicate directory "[[PWD]]/test5/include/u"
+// CHECK-NEXT: #include "..." search starts here:
+// CHECK-NEXT: #include <...> search starts here:
+// CHECK-NEXT: [[PWD]]/test5/include/u
+// CHECK-NEXT: [[PWD]]/test5/include/v
+// CHECK-NEXT: [[PWD]]/test5/include/w
+// CHECK-NEXT: [[PWD]]/test5/include/x
+// CHECK-NEXT: [[PWD]]/test5/include/y
+// CHECK-NEXT: [[PWD]]/test5/include/z
+// CHECK-NEXT: End of search list.
 
-#--- include/q/c.h
-#error 'include/q/c.h' should not have been included!
+#--- test5/include/u/a.h
 
-#--- include/q/d.h
-#error 'include/q/d.h' should not have been included!
+#--- test5/include/v/a.h
+#error 'test5/include/v/a.h' should not have been included!
 
-#--- include/q/e.h
-#error 'include/q/e.h' should not have been included!
+#--- test5/include/v/b.h
 
-#--- include/q/f.h
-#error 'include/q/f.h' should not have been included!
+#--- test5/include/w/a.h
+#error 'test5/include/w/a.h' should not have been included!
 
-#--- include/q/g.h
-#error 'include/q/g.h' should not have been included!
+#--- test5/include/w/b.h
+#error 'test5/include/w/b.h' should not have been included!
 
-#--- include/q/h.h
-#error 'include/q/h.h' should not have been included!
+#--- test5/include/w/c.h
 
-#--- include/q/i.h
-#error 'include/q/i.h' should not have been included!
+#--- test5/include/x/a.h
+#error 'test5/include/x/a.h' should not have been included!
 
-#--- include/q/j.h
-#error 'include/q/j.h' should not have been included!
+#--- test5/include/x/b.h
+#error 'test5/include/x/b.h' should not have been included!
 
-#--- include/q/k.h
+#--- test5/include/x/c.h
+#error 'test5/include/x/c.h' should not have been included!
 
-#--- include/p/a.h
-#error 'include/p/a.h' should not have been included!
+#--- test5/include/x/d.h
 
-#--- include/p/b.h
-#error 'include/p/b.h' should not have been included!
+#--- test5/include/y/a.h
+#error 'test5/include/y/a.h' should not have been included!
 
-#--- include/p/c.h
-#error 'include/p/c.h' should not have been included!
+#--- test5/include/y/b.h
+#error 'test5/include/y/b.h' should not have been included!
 
-#--- include/p/d.h
-#error 'include/p/d.h' should not have been included!
+#--- test5/include/y/c.h
+#error 'test5/include/y/c.h' should not have been included!
 
-#--- include/p/e.h
-#error 'include/p/e.h' should not have been included!
+#--- test5/include/y/d.h
+#error 'test5/include/y/d.h' should not have been included!
 
-#--- include/p/f.h
-#error 'include/p/f.h' should not have been included!
+#--- test5/include/y/e.h
 
-#--- include/p/g.h
-#error 'include/p/g.h' should not have been included!
+#--- test5/include/z/a.h
+#error 'test5/include/z/a.h' should not have been included!
 
-#--- include/p/h.h
-#error 'include/p/h.h' should not have been included!
+#--- test5/include/z/b.h
+#error 'test5/include/z/b.h' should not have been included!
 
-#--- include/p/i.h
-#error 'include/p/i.h' should not have been included!
+#--- test5/include/z/c.h
+#error 'test5/include/z/c.h' should not have been included!
 
-#--- include/p/j.h
-#error 'include/p/j.h' should not have been included!
+#--- test5/include/z/d.h
+#error 'test5/include/z/d.h' should not have been included!
 
-#--- include/p/k.h
-#error 'include/p/k.h' should not have been included!
+#--- test5/include/z/e.h
+#error 'test5/include/z/e.h' should not have been included!
 
-#--- include/p/l.h
+#--- test5/include/z/f.h

>From d3a682bda7b3ae9344a6a29e43f0feb5394747dc Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Thu, 24 Oct 2024 14:49:27 -0700
Subject: [PATCH 8/9] WIP to be squashed; remove FIXME comments to be followed
 up separately.

---
 clang/lib/Driver/ToolChains/MSVC.cpp | 15 ---------------
 clang/lib/Lex/HeaderSearch.cpp       |  5 -----
 clang/lib/Lex/InitHeaderSearch.cpp   |  6 ------
 3 files changed, 26 deletions(-)

diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 46217476f8b1e4..618ae62576f522 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -636,8 +636,6 @@ void MSVCToolChain::AddSystemIncludeWithSubfolder(
 
 void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                               ArgStringList &CC1Args) const {
-  // FIXME: Options explicitly present in the command line should still be
-  // FIXME: processed. E.g., the /imsvc option handled below.
   if (DriverArgs.hasArg(options::OPT_nostdinc))
     return;
 
@@ -646,12 +644,6 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                   "include");
   }
 
-  // FIXME: According to commit fd3e1ad0ce073e67c21833e56b74105ce8339ce3, the
-  // FIXME: -imsvc option is intended to behave as though the specified path
-  // FIXME: was present in %INCLUDE%, but these paths are getting added earlier
-  // FIXME: than %INCLUDE% paths with the DIA SDK paths being added in between.
-  // FIXME: Is this the intended behavior? Support for adding the DIA SDK paths
-  // FIXME: was introduced later via commit 951f362e2560fe1c9c05f487107fd9882d45d867.
   // Add %INCLUDE%-like directories from the -imsvc flag.
   for (const auto &Path : DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc))
     addSystemInclude(DriverArgs, CC1Args, Path);
@@ -688,13 +680,6 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   // 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.
-  // FIXME: MSVC handles paths specified by the INCLUDE environment variable
-  // FIXME: as user paths unless an external include path specified by a
-  // FIXME: /external:I or /external:env option or the EXTERNAL_INCLUDE
-  // FIXME: environment variable is a prefix match in which case, warnings
-  // FIXME: are issued subject to the /external:Wn option. For now, all paths
-  // FIXME: specified in INCLUDE are handled as external paths which results
-  // FIXME: in them being handled as system paths.
   if (!DriverArgs.getLastArg(options::OPT__SLASH_vctoolsdir,
                              options::OPT__SLASH_winsysroot)) {
     bool Found = AddExternalIncludesFromEnv("INCLUDE");
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index b03e23f843f503..8826ab449df493 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1112,11 +1112,6 @@ OptionalFileEntryRef HeaderSearch::LookupFile(
     if (HFI.DirInfo == SrcMgr::C_User && InUserSpecifiedSystemFramework)
       HFI.DirInfo = SrcMgr::C_System;
 
-    // FIXME: If /external:anglebrackets is enabled and the inclusion was
-    // FIXME: with angle brackets, then promote the characteristic.
-    // if (isAngled && <external:anglebrackets>)
-    //   HFI.DirInfo = SrcMgr::C_System; // Or C_External?
-
     // If the filename matches a known system header prefix, override
     // whether the file is a system header.
     for (unsigned j = SystemHeaderPrefixes.size(); j; --j) {
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index a2a638cb44eaea..a3bd7d5db775aa 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -155,7 +155,6 @@ bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group,
     Type = SrcMgr::C_ExternCSystem;
   } else {
     // Group in External, ExternalAfter, System, (Obj)C(XX)System, After.
-    // FIXME: Assert that the group is known?
     Type = SrcMgr::C_System;
   }
 
@@ -404,7 +403,6 @@ static unsigned RemoveDuplicates(const LangOptions &Lang,
     // to ensure that #include and #include_next directives produce the same
     // results as these other compilers.
     //
-    // FIXME: The following comment isn't accurate.
     // GCC and MSVC both prune duplicate user search locations that follow a
     // previous matching user search location. Both compilers also prune user
     // search locations that are also present as system search locations
@@ -518,10 +516,6 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   RemoveDuplicates(Lang, SearchList, 0, 0, Verbose);
   unsigned EndQuoted = SearchList.size();
 
-  // FIXME: GCC ignores the last path for quoted inclusion when it matches the
-  // FIXME: first path for angled inclusion. This is observable when using
-  // FIXME: #include_next.
-
   // Add search paths for angled inclusion next. Note that user paths and
   // external paths may be interleaved; though external paths are treated like
   // system paths, they are not reordered to the end of the search list.

>From 7bf660284c4469dcf24a4731a6c10451e9659252 Mon Sep 17 00:00:00 2001
From: Tom Honermann <tom.honermann at intel.com>
Date: Fri, 25 Oct 2024 09:58:37 -0700
Subject: [PATCH 9/9] WIP to be squashed; Restore INCLUDE/EXTERNAL_INCLUDE
 ordering relative to builtin paths.

Reverts paths in INCLUDE and EXTERNAL_INCLUDE to being System paths instead of
ExternalAfter paths to restore order with Clang's builtin include paths. This
now requires suppressing the removal of previous user paths that duplicate
system paths in Microsoft compatibility mode; which seems like the right
behavior anyway. Minor release note updates.
---
 clang/docs/ReleaseNotes.rst            | 14 ++++++------
 clang/include/clang/Driver/ToolChain.h |  7 ++++--
 clang/lib/Driver/ToolChain.cpp         | 30 ++++++++++++++++++++++----
 clang/lib/Driver/ToolChains/Clang.cpp  |  9 +-------
 clang/lib/Driver/ToolChains/MSVC.cpp   | 16 ++------------
 clang/lib/Lex/InitHeaderSearch.cpp     |  6 ++++--
 clang/test/Driver/cl-include.c         | 18 +++++++++-------
 7 files changed, 55 insertions(+), 45 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 462ddd65a24c19..c78775c2aa9319 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -644,8 +644,8 @@ Windows Support
   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 duplicate across ``/I`` options
-  and the ``%INCLUDE%`` environment variable.
+  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.
@@ -663,17 +663,17 @@ Windows Support
     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
+  - 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
+    duplicate an earlier path in the ``INCLUDE`` environment variable are
     ignored.
 
-  - Paths specified by the ``%EXTERNAL_INCLUDE%`` environment variable are
+  - 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%``
+    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
diff --git a/clang/include/clang/Driver/ToolChain.h b/clang/include/clang/Driver/ToolChain.h
index 645064fcf37d8f..6687c3b3833fde 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/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,10 +240,13 @@ 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 void addExternalIncludesFromEnv(const llvm::opt::ArgList &DriverArgs,
+  static bool addExternalIncludesFromEnv(const llvm::opt::ArgList &DriverArgs,
                                          llvm::opt::ArgStringList &CC1Args,
                                          StringRef Var);
 
@@ -250,7 +254,6 @@ class ToolChain {
                             const Twine &C = "", const Twine &D = "");
   ///@}
 
-public:
   virtual ~ToolChain();
 
   // Accessors
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index a6bbccad014eb8..62f4d7a627e11e 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1265,6 +1265,23 @@ 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,
@@ -1276,16 +1293,21 @@ void ToolChain::addExternCSystemIncludeIfExists(const ArgList &DriverArgs,
   }
 }
 
-/// Utility function to add a list of directories specified in an environment
-/// variable to the end of the external include path list for CC1.
-/*static*/ void ToolChain::addExternalIncludesFromEnv(const ArgList &DriverArgs,
+/// 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);
-    addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
+    if (!Dirs.empty()) {
+      addExternalAfterIncludes(DriverArgs, CC1Args, Dirs);
+      return true;
+    }
   }
+  return false;
 }
 
 /*static*/ std::string ToolChain::concat(StringRef Path, const Twine &A,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index b9f278ae2d118a..3a8d4283c9d015 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -1180,14 +1180,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
       continue;
     } else if (A->getOption().matches(options::OPT_iexternal_env_EQ)) {
       A->claim();
-      if (auto Val = llvm::sys::Process::GetEnv(A->getValue())) {
-        SmallVector<StringRef, 8> Dirs;
-        StringRef(*Val).split(Dirs, ";", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
-        for (const auto &Dir : Dirs) {
-          CmdArgs.push_back("-iexternal-after");
-          CmdArgs.push_back(Args.MakeArgString(Dir));
-        }
-      }
+      ToolChain::addExternalIncludesFromEnv(Args, CmdArgs, A->getValue());
       continue;
     }
 
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 618ae62576f522..1590f572da97cb 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -664,26 +664,14 @@ void MSVCToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
   if (DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
-  auto AddExternalIncludesFromEnv = [&](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()) {
-        addExternalIncludesFromEnv(DriverArgs, CC1Args, Var);
-        return true;
-      }
-    }
-    return false;
-  };
-
   // 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 = AddExternalIncludesFromEnv("INCLUDE");
-    Found |= AddExternalIncludesFromEnv("EXTERNAL_INCLUDE");
+    bool Found = addSystemIncludesFromEnv(DriverArgs, CC1Args, "INCLUDE");
+    Found |= addSystemIncludesFromEnv(DriverArgs, CC1Args, "EXTERNAL_INCLUDE");
     if (Found)
       return;
   }
diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp
index a3bd7d5db775aa..73f98a121db2dd 100644
--- a/clang/lib/Lex/InitHeaderSearch.cpp
+++ b/clang/lib/Lex/InitHeaderSearch.cpp
@@ -560,9 +560,11 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) {
   // the system search paths. This duplicate removal is necessary to ensure
   // that header lookup in #include_next directives doesn't resolve to the
   // same file. This may result in earlier user and external paths being
-  // removed, and thus requires updating the EndAngled index.
+  // removed, and thus requires updating the EndAngled index. Removal of
+  // earlier paths is not done in Microsoft compatibility mode for system paths.
   unsigned NonSystemRemoved =
-      RemoveDuplicates(Lang, SearchList, EndQuoted, EndAngled, Verbose);
+      RemoveDuplicates(Lang, SearchList, EndQuoted, EndAngled, Verbose,
+                       /*RemovePreviousAllowed*/ !Lang.MSVCCompat);
   EndAngled -= NonSystemRemoved;
 
   Headers.SetSearchPaths(extractLookups(SearchList), EndQuoted, EndAngled,
diff --git a/clang/test/Driver/cl-include.c b/clang/test/Driver/cl-include.c
index e31c152eb54020..df5e2aeb4d4a1a 100644
--- a/clang/test/Driver/cl-include.c
+++ b/clang/test/Driver/cl-include.c
@@ -8,31 +8,33 @@
 // NOBUILTIN-NOT: "-internal-isystem" "{{.*lib.*clang.*include}}"
 
 // RUN: env INCLUDE=/my/system/inc env EXTERNAL_INCLUDE=/my/system/inc2 %clang_cl -### -- %s 2>&1 | FileCheck %s --check-prefix=STDINC
-// STDINC: "-iexternal-after" "/my/system/inc"
-// STDINC: "-iexternal-after" "/my/system/inc2"
+// STDINC: "-internal-isystem" "{{.*lib.*clang.*include}}"
+// 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 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: "-iexternal-after" "/my/system/inc"
-// NOSTDINC-NOT: "-iexternal-after" "/my/system/inc2"
 // NOSTDINC-NOT: "-internal-isystem" "{{.*lib.*clang.*include}}"
 // NOSTDINC-NOT: "-internal-isystem" "/my/other/inc"
+// NOSTDINC-NOT: "-internal-isystem" "/my/system/inc"
+// NOSTDINC-NOT: "-internal-isystem" "/my/system/inc2"
 
 // /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: "-external-after" "/my/system/inc"
-// SLASHX-NOT: "-external-after" "/my/system/inc2"
+// SLASHX-NOT: "-internal-isystem" "/my/system/inc"
+// SLASHX-NOT: "-internal-isystem" "/my/system/inc2"
 // SLASHX: "-iexternal-after" "/my/other/inc2"
 // SLASHX: "-internal-isystem" "{{.*lib.*clang.*include}}"
 // SLASHX: "-internal-isystem" "/my/other/inc"
 
-// /winsysroot suppresses %EXTERNAL_INCLUDE% but not -imsvc dirs or /external: flags.
-// RUN: 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
+// /winsysroot suppresses %INCLUDE% and %EXTERNAL_INCLUDE% but not -imsvc dirs or /external: flags.
+// RUN: env INCLUDE=/my/system/inc env EXTERNAL_INCLUDE=/my/system/inc2 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-NOT: "/my/system/inc2"
 // SYSROOT: "-iexternal-after" "/my/other/inc2"
 // SYSROOT: "-internal-isystem" "/my/other/inc"
 // SYSROOT: "-internal-isystem" "/foo{{.*}}"



More information about the cfe-commits mailing list