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

[tahonermann]

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).

[llvmbot]

@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Tom Honermann (tahonermann)

Changes

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 -fms-compatibility or with the clang-cl driver).

---

Full diff: https://github.com/llvm/llvm-project/pull/105738.diff

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+14)
• (modified) clang/lib/Lex/InitHeaderSearch.cpp (+12-9)
• (added) clang/test/Driver/header-search-duplicates.c (+74)
• (added) clang/test/Driver/microsoft-header-search-duplicates.c (+90)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5c156a9c073a9c..d9c96d8902efa9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -363,6 +363,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..a4cba469cd7e31 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..2480d9f24d8200
--- /dev/null
+++ b/clang/test/Driver/header-search-duplicates.c
@@ -0,0 +1,74 @@
+// 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 to validate that the gcc compatible behavior is enabled
+// by default. The -nostdinc option is used to suppress default search paths to
+// ease testing.
+// RUN: %clang \
+// RUN:     -v -fsyntax-only -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..1b73eeceb37eba
--- /dev/null
+++ b/clang/test/Driver/microsoft-header-search-duplicates.c
@@ -0,0 +1,90 @@
+// 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 to validate that the Microsoft compatible behavior is
+// enabled when the -fms-compatibility option is used. The -nostdinc option
+// is used to suppress default search paths to ease testing.
+// RUN: %clang \
+// RUN:     -v -fsyntax-only -fms-compatibility -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 to validate that the Microsoft compatible behavior
+// is enabled by default. The -nobuiltininc option is used instead of -nostdinc
+// or /X because the latter two suppress all system include paths (including
+// those specified by -isystem and -imsvc). The -imsvc option is used because
+// the -nobuiltininc option suppresses search paths specified by -isystem.
+// RUN: %clang_cl \
+// RUN:     -v -fsyntax-only -nobuiltininc \
+// 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

[github-actions[bot]]

:warning: C/C++ code formatter, clang-format found issues in your code. :warning:

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions h,cpp,c -- clang/test/Driver/header-search-duplicates.c clang/test/Driver/microsoft-header-search-duplicates.c clang/test/Frontend/external-include-diags-junction-windows.c clang/test/Frontend/external-include-diags-normalized-windows.c clang/test/Frontend/external-include-diags-normalized.c clang/test/Frontend/external-include-diags-space.c clang/test/Frontend/external-include-diags-symlink-dir-windows.c clang/test/Frontend/external-include-diags-symlink-dir.c clang/test/Frontend/external-include-diags-symlink-file-windows.c clang/test/Frontend/external-include-diags-symlink-file.c clang/include/clang/Driver/ToolChain.h clang/include/clang/Lex/HeaderSearch.h clang/include/clang/Lex/HeaderSearchOptions.h clang/lib/Driver/Driver.cpp clang/lib/Driver/ToolChain.cpp clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Driver/ToolChains/MSVC.cpp clang/lib/Frontend/CompilerInvocation.cpp clang/lib/Lex/HeaderSearch.cpp clang/lib/Lex/InitHeaderSearch.cpp clang/lib/Lex/PPDirectives.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTWriter.cpp clang/test/Driver/cl-include.c clang/test/Driver/cl-options.c clang/test/Preprocessor/microsoft-header-search-fail.c clang/test/Preprocessor/microsoft-header-search.c llvm/include/llvm/Option/ArgList.h llvm/lib/Option/ArgList.cpp

:warning: The reproduction instructions above might return results for more than one PR in a stack if you are using a stacked PR workflow. You can limit the results by changing origin/main to the base branch/commit you want to compare against. :warning:

View the diff from clang-format here.

diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index d031b05e7..78be080ab 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -1456,8 +1456,7 @@ void ToolChain::addSystemFrameworkIncludes(const ArgList &DriverArgs,
 /// Utility function to add a list of system include directories to CC1.
 void ToolChain::addSystemIncludes(const ArgList &DriverArgs,
                                   ArgStringList &CC1Args,
-                                  ArrayRef<StringRef> Paths,
-                                  bool Internal) {
+                                  ArrayRef<StringRef> Paths, bool Internal) {
   for (const auto &Path : Paths) {
     if (Internal)
       CC1Args.push_back("-internal-isystem");

[tahonermann]

It took me a few tries to get the tests tweaked just right for the clang-cl driver to pass on Windows, but the tests now pass and this PR is now ready for review.

[tahonermann]

Sigh, reverted to draft again. I noticed a vestigial file in both test cases, a misplaced #include_next directive, and on further investigation discovered that there is an additional divergence in the MSVC behavior that the patch doesn't address (it looks like a user include path that is later duplicated as a system include path is promoted to a system include path in place such that its position is preserved relative to other system include paths; but I need to investigate further).

[zygoloid]

This choice seems like a question of how the driver's command line is interpreted, rather than a language mode, so I wonder if it makes sense for it to consider -fms-compatibility at all or whether this should just be based on the driver mode. Do you have rationale for this also changing under -fms-compatibility?

[tahonermann]

This choice seems like a question of how the driver's command line is interpreted, rather than a language mode, so I wonder if it makes sense for it to consider -fms-compatibility at all or whether this should just be based on the driver mode. Do you have rationale for this also changing under -fms-compatibility?

@zygoloid, that is a fair question. My original intent had been to make the behavior contingent on driver mode. I switched to tying it to -fms-compatibility because other preprocessor behavior is contingent on that option and because that would be a less invasive change. Duplicate search path pruning is currently done by cc1 and I don't think that should be moved to the driver. I could be mistaken, but I don't think driver mode is exposed to cc1 (and that is probably a good thing). We could introduce a new cc1 option to opt-in to Microsoft style search path pruning. I'm not opposed to that.

[zygoloid]

I've been pondering this. On the one hand, people switching from the cl.exe-compatible driver to the GCC-compatible driver might want the MSVC interpretation of the path flags. On the other hand, people enabling -fms-compatibility to accept code written against MSVC in a different environment might be surprised if other command-line flags behave differently (especially include paths unrelated to those files). On the third hand, this only affects cases where the same path is specified in more than one kind of list, which seems like it's probably rare. On the fourth hand, IIRC the choice of whether header search for a relative path looks in the directory of includers as well as the directory of the current file already depends on -fms-compatibility, and this seems sort of similar. So yeah, I dunno what we should do; it doesn't seem clear to me.

[tahonermann]

@zygoloid,

On the fourth hand, IIRC the choice of whether header search for a relative path looks in the directory of includers as well as the directory of the current file already depends on -fms-compatibility, and this seems sort of similar.

I confirmed the search of the include stack directories is controlled by -fms-compatibility. The relevant code is https://github.com/llvm/llvm-project/blob/f58ce1152703ca753794b8cef36da30bd2668d0f/clang/lib/Lex/PPDirectives.cpp#L1003-L1013.

So yeah, I dunno what we should do; it doesn't seem clear to me.

Yup. My intuition has been to follow the existing precedent unless/until motivation to do otherwise becomes apparent.

[rnk]

I would stick with fms-compatibility being the setting here rather than plumbing the driver mode through. Over the years, many build systems have been adapted to work with clang[++] and cl without going through clang-cl.

[tahonermann]

There is some interesting MSVC behavior that I'm not planning to replicate with the changes being done for this issue but that I thought are worth documenting (here at least, perhaps in Clang docs as well).

MSVC documentation for the /external suite of options includes the /external:Wn option (where Wn is one of W0, W1, W2,W3, or W4) to specify the warning level that is used for "external" header search paths; paths specified with the /external:I or /external:env option, the %EXTERNAL_INCLUDE% environment variable, or included with a angle bracket enclosed path (#include <file.h>) when the /external:anglebrackets option is enabled. The warning levels are meaningless to Clang, so Clang currently maps /external:W0 to -Wno-system-headers and the rest to -Wsystem-headers. Clang does not yet implement /external:anglebrackets. This all seems fine.

There are two interesting behaviors that MSVC exhibits related to the /I and %INCLUDE% environment variable.

1. By default, paths specified by /I or %INCLUDE% are treated as user paths; whether warnings are issued for them is subject to use of one of the warning level options like /Wall and /W4; the /external:Wn option has no effect on them.
2. However, when a path specified by /external:I, /external:env, or %EXTERNAL_INCLUDE% matches the beginning of a path in a /I or %INCLUDE% path, then the /external:Wn specified warning level is applied to that path. The path match does not have to occur at a path component boundary (unless the external path ends in a path separator). Matched paths are treated like system paths.

In the following example, each header file has code that triggers MSVC warning C4245; a warning that is issued at warning level 4.

> type incbar\a.h
const unsigned int sysinc1 = -1;

> type incbaz\b.h
const unsigned int sysinc2 = -1;

> type incfoo\c.h
const unsigned int extinc = -1;

> type t.cpp
#include <a.h>
#include <b.h>
#include <c.h>

> set INCLUDE=incbar;incbaz;incfoo

# No warnings issued by default.
> cl /nologo /c t.cpp
t.cpp

# Warnings issued with `/W4`.
> cl /nologo /c /W4 t.cpp
t.cpp
incbar\a.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch
incbaz\b.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch
incfoo\c.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch

# No warnings issued with `/external:W4`.
> cl /nologo /c /external:W4 t.cpp
t.cpp

# Warnings issued for all three search directories with `/external:W4` and `/external:I inc`.
# Not shown here, but if a `inc` directory existed, it would also be searched for header files.
> cl /nologo /c /external:W4 /external:I inc t.cpp
t.cpp
incbar\a.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch
incbaz\b.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch
incfoo\c.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch

# Warnings issued for only `incbar/a.h` and `incbaz.h` with `/external:W4` and `/external:I incb`.
# Not shown here, but if a `incb` directory existed, it would also be searched for header files.
> cl /nologo /c /external:W4 /external:I incb t.cpp
t.cpp
incbar\a.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch
incbaz\b.h(1): warning C4245: 'initializing': conversion from 'int' to 'unsigned int', signed/unsigned mismatch

# No warnings issued when the external include path ends with a path separator that doesn't match another path.
# Not shown here, but if a `incb` directory existed, it would be searched for header files.
> cl /nologo /c /external:W4 /external:I incb\ t.cpp
t.cpp

My intent with changes made for this issue is to (continue to) treat all paths specified by /I as user paths and all paths specified by /external:I, /external:env, %INCLUDE%, or %EXTERNAL_INCLUDE% as system paths.

Ideally, I think we would do the following at some point to improve compatibility with MSVC.

1. Treat paths specified by %INCLUDE% as user paths by default.
2. After building the header search path in InitHeaderSearch::Realize(), alter the SrcMgr::CharacteristicKind value stored in the Lookup member of DirectoryLookupInfo in the IncludePath member of InitHeaderSearch to indicate a system path for each path that is a prefix match for an external path (where the prefix need not match on a path component boundary).

[tahonermann]

This PR is finally ready for review. I would especially appreciate it if @rnk and @nico took a close look. Please add other known stakeholders.

[AaronBallman]

My intent with changes made for this issue is to (continue to) treat all paths specified by /I as user paths and all paths specified by /external:I, /external:env, %INCLUDE%, or %EXTERNAL_INCLUDE% as system paths.

I think this option is the least disruptive.

Ideally, I think we would do the following at some point to improve compatibility with MSVC.

I'm not opposed, but I am concerned about the potential to subtly break user code that's relying on our current search path behavior. We may need to find some clever diagnostics for cases where lookup would have previously succeeded or found a different file.

[tahonermann]

Thanks, @AaronBallman,

Ideally, I think we would do the following at some point to improve compatibility with MSVC.

I'm not opposed, but I am concerned about the potential to subtly break user code that's relying on our current search path behavior. We may need to find some clever diagnostics for cases where lookup would have previously succeeded or found a different file.

The changes I was suggesting in that comment would not change what files are found; it would only affect whether the file was to be treated as a user or system header. That being said, the changes in this PR do affect file resolution, but only in cases where the same path is specified multiple times.

[tahonermann]

Friendly ping for additional code review. @rnk, @nico, @zmodem, @majnemer, @zygoloid.

The Windows build is failing spuriously. I've rebased and force pushed multiple times to try to get a clean build, but it keeps failing during git checkout.

[AaronBallman]

Thanks, @AaronBallman,

Ideally, I think we would do the following at some point to improve compatibility with MSVC.

I'm not opposed, but I am concerned about the potential to subtly break user code that's relying on our current search path behavior. We may need to find some clever diagnostics for cases where lookup would have previously succeeded or found a different file.

The changes I was suggesting in that comment would not change what files are found; it would only affect whether the file was to be treated as a user or system header.

Doesn't that impact whether a header is found via "" or <> syntax? e.g., if something went from a user header to a system header, I thought that meant the search order would then change depending on which include syntax you used?

[tahonermann]

Doesn't that impact whether a header is found via "" or <> syntax? e.g., if something went from a user header to a system header, I thought that meant the search order would then change depending on which include syntax you used?

No, those are orthogonal concerns, but there is unfortunate terminology overlap. Options like -isystem nominate system include paths and headers found within those paths are treated as system headers. However, a header found via another include path can still be considered a system header. Consider a header file that contains #pragma GCC system_header; such a header file is considered a system header, but doesn't influence how header file inclusion is resolved.

[AaronBallman]

Doesn't that impact whether a header is found via "" or <> syntax? e.g., if something went from a user header to a system header, I thought that meant the search order would then change depending on which include syntax you used?

No, those are orthogonal concerns, but there is unfortunate terminology overlap. Options like -isystem nominate system include paths and headers found within those paths are treated as system headers. However, a header found via another include path can still be considered a system header. Consider a header file that contains #pragma GCC system_header; such a header file is considered a system header, but doesn't influence how header file inclusion is resolved.

Okay, thank you! Then this seem good to me.

[tahonermann]

Further internal testing has indicated that there will likely be backward compatibility problems in practice with the implemented approach of differentiating behavior based on -fms-compatibility. I'm therefore going to look into adding a new -cc1 option, -fms-header-search, to control this behavior. The new option will only be enabled by default for clang-cl.

Opinions on whether the current -fms-compatibility controlled behavior to search the directories of the include stack for #include "xxx" headers should be switched to this new option is welcome. I tend to think it should be.

[AaronBallman]

Opinions on whether the current -fms-compatibility controlled behavior to search the directories of the include stack for #include "xxx" headers should be switched to this new option is welcome. I tend to think it should be.

My intuition is that it should be switched to the new option; we want -fms-compatibility to mean "be compatible with MSVC" and that shouldn't require additional flags except in exceptional cases. This doesn't feel like an exceptional case to me.

[tahonermann]

I've been continuing to work on this, but have yet to get all of our internal tests to pass. I've reverted the PR to a draft pending successful internal testing.