[ClangImporter] clang's -iframework comes before builtin usr/local/include, but Swift's -Fsystem comes after

When Swift passes search paths to clang, it does so directly into the HeaderSearch. That means that those paths get ordered inconsistently compared to the equivalent clang flag, and causes inconsistencies when building clang modules with clang and with Swift. Instead of touching the HeaderSearch directly, pass Swift search paths as driver flags, just do them after the -Xcc ones.

Swift doesn't have a way to pass a search path to clang as -isystem, only as -I which usually isn't the right flag. Add an -Isystem Swift flag so that those paths can be passed to clang as -isystem.

rdar://93951328

Author: ian-twilightcoder

include/swift/AST/SearchPathOptions.h

@@ -311,18 +311,16 @@ class SearchPathOptions {
   friend class ASTContext;
 
 public:
-  struct FrameworkSearchPath {
+  struct SearchPath {
     std::string Path;
     bool IsSystem = false;
-    FrameworkSearchPath(StringRef path, bool isSystem)
-    : Path(path), IsSystem(isSystem) {}
+    SearchPath(StringRef path, bool isSystem)
+        : Path(path), IsSystem(isSystem) {}
 
-    friend bool operator ==(const FrameworkSearchPath &LHS,
-                            const FrameworkSearchPath &RHS) {
+    friend bool operator==(const SearchPath &LHS, const SearchPath &RHS) {
       return LHS.Path == RHS.Path && LHS.IsSystem == RHS.IsSystem;
     }
-    friend bool operator !=(const FrameworkSearchPath &LHS,
-                            const FrameworkSearchPath &RHS) {
+    friend bool operator!=(const SearchPath &LHS, const SearchPath &RHS) {
       return !(LHS == RHS);
     }
   };
@@ -332,23 +330,23 @@ class SearchPathOptions {
 
   /// Path to the SDK which is being built against.
   ///
-  /// Must me modified through setter to keep \c SearchPathLookup in sync.
+  /// Must be modified through setter to keep \c Lookup in sync.
   std::string SDKPath;
 
   /// Path(s) which should be searched for modules.
   ///
-  /// Must me modified through setter to keep \c SearchPathLookup in sync.
-  std::vector<std::string> ImportSearchPaths;
+  /// Must be modified through setter to keep \c Lookup in sync.
+  std::vector<SearchPath> ImportSearchPaths;
 
   /// Path(s) which should be searched for frameworks.
   ///
-  /// Must me modified through setter to keep \c SearchPathLookup in sync.
-  std::vector<FrameworkSearchPath> FrameworkSearchPaths;
+  /// Must be modified through setter to keep \c Lookup in sync.
+  std::vector<SearchPath> FrameworkSearchPaths;
 
   /// Paths to search for stdlib modules. One of these will be
   /// compiler-relative.
   ///
-  /// Must me modified through setter to keep \c SearchPathLookup in sync.
+  /// Must be modified through setter to keep \c Lookup in sync.
   std::vector<std::string> RuntimeLibraryImportPaths;
 
   /// When on Darwin the framework paths that are implicitly imported.
@@ -369,17 +367,16 @@ class SearchPathOptions {
 
   /// Add a single import search path. Must only be called from
   /// \c ASTContext::addSearchPath.
-  void addImportSearchPath(StringRef Path, llvm::vfs::FileSystem *FS) {
-    ImportSearchPaths.push_back(Path.str());
-    Lookup.searchPathAdded(FS, ImportSearchPaths.back(),
-                           ModuleSearchPathKind::Import, /*isSystem=*/false,
+  void addImportSearchPath(SearchPath Path, llvm::vfs::FileSystem *FS) {
+    ImportSearchPaths.push_back(Path);
+    Lookup.searchPathAdded(FS, ImportSearchPaths.back().Path,
+                           ModuleSearchPathKind::Import, Path.IsSystem,
                            ImportSearchPaths.size() - 1);
   }
 
   /// Add a single framework search path. Must only be called from
   /// \c ASTContext::addSearchPath.
-  void addFrameworkSearchPath(FrameworkSearchPath NewPath,
-                              llvm::vfs::FileSystem *FS) {
+  void addFrameworkSearchPath(SearchPath NewPath, llvm::vfs::FileSystem *FS) {
     FrameworkSearchPaths.push_back(NewPath);
     Lookup.searchPathAdded(FS, FrameworkSearchPaths.back().Path,
                            ModuleSearchPathKind::Framework, NewPath.IsSystem,
@@ -448,21 +445,21 @@ class SearchPathOptions {
     SysRoot = sysroot;
   }
 
-  ArrayRef<std::string> getImportSearchPaths() const {
+  ArrayRef<SearchPath> getImportSearchPaths() const {
     return ImportSearchPaths;
   }
 
-  void setImportSearchPaths(std::vector<std::string> NewImportSearchPaths) {
+  void setImportSearchPaths(std::vector<SearchPath> NewImportSearchPaths) {
     ImportSearchPaths = NewImportSearchPaths;
     Lookup.searchPathsDidChange();
   }
 
-  ArrayRef<FrameworkSearchPath> getFrameworkSearchPaths() const {
+  ArrayRef<SearchPath> getFrameworkSearchPaths() const {
     return FrameworkSearchPaths;
   }
 
-  void setFrameworkSearchPaths(
-      std::vector<FrameworkSearchPath> NewFrameworkSearchPaths) {
+  void
+  setFrameworkSearchPaths(std::vector<SearchPath> NewFrameworkSearchPaths) {
     FrameworkSearchPaths = NewFrameworkSearchPaths;
     Lookup.searchPathsDidChange();
   }
@@ -597,8 +594,7 @@ class SearchPathOptions {
       llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) const;
 
 private:
-  static StringRef
-  pathStringFromFrameworkSearchPath(const FrameworkSearchPath &next) {
+  static StringRef pathStringFromSearchPath(const SearchPath &next) {
     return next.Path;
   };
 
@@ -609,17 +605,16 @@ class SearchPathOptions {
     using llvm::hash_combine;
     using llvm::hash_combine_range;
 
-    using FrameworkPathView = ArrayRefView<FrameworkSearchPath, StringRef,
-                                           pathStringFromFrameworkSearchPath>;
-    FrameworkPathView frameworkPathsOnly{FrameworkSearchPaths};
+    using SearchPathView =
+        ArrayRefView<SearchPath, StringRef, pathStringFromSearchPath>;
+    SearchPathView importPathsOnly{ImportSearchPaths};
+    SearchPathView frameworkPathsOnly{FrameworkSearchPaths};
 
     return hash_combine(SDKPath,
-                        hash_combine_range(ImportSearchPaths.begin(),
-                                           ImportSearchPaths.end()),
-                        hash_combine_range(VFSOverlayFiles.begin(),
-                                           VFSOverlayFiles.end()),
-                        // FIXME: Should we include the system-ness of framework
+                        // FIXME: Should we include the system-ness of
                         // search paths too?
+                        hash_combine_range(importPathsOnly.begin(), importPathsOnly.end()),
+                        hash_combine_range(VFSOverlayFiles.begin(), VFSOverlayFiles.end()),
                         hash_combine_range(frameworkPathsOnly.begin(),
                                            frameworkPathsOnly.end()),
                         hash_combine_range(LibrarySearchPaths.begin(),

include/swift/Frontend/Frontend.h

@@ -187,7 +187,8 @@ class CompilerInvocation {
     return ClangImporterOpts.ClangScannerModuleCachePath;
   }
 
-  void setImportSearchPaths(const std::vector<std::string> &Paths) {
+  void setImportSearchPaths(
+      const std::vector<SearchPathOptions::SearchPath> &Paths) {
     SearchPathOpts.setImportSearchPaths(Paths);
   }
 
@@ -196,16 +197,16 @@ class CompilerInvocation {
     SearchPathOpts.DeserializedPathRecoverer = obfuscator;
   }
 
-  ArrayRef<std::string> getImportSearchPaths() const {
+  ArrayRef<SearchPathOptions::SearchPath> getImportSearchPaths() const {
     return SearchPathOpts.getImportSearchPaths();
   }
 
   void setFrameworkSearchPaths(
-             const std::vector<SearchPathOptions::FrameworkSearchPath> &Paths) {
+      const std::vector<SearchPathOptions::SearchPath> &Paths) {
     SearchPathOpts.setFrameworkSearchPaths(Paths);
   }
 
-  ArrayRef<SearchPathOptions::FrameworkSearchPath> getFrameworkSearchPaths() const {
+  ArrayRef<SearchPathOptions::SearchPath> getFrameworkSearchPaths() const {
     return SearchPathOpts.getFrameworkSearchPaths();
   }
 

include/swift/Option/Options.td

@@ -338,6 +338,11 @@ def I : JoinedOrSeparate<["-"], "I">,
 def I_EQ : Joined<["-"], "I=">, Flags<[FrontendOption, ArgumentIsPath]>,
   Alias<I>;
 
+def Isystem : Separate<["-"], "Isystem">,
+  Flags<[FrontendOption, ArgumentIsPath, SwiftSymbolGraphExtractOption,
+         SwiftSynthesizeInterfaceOption]>,
+  HelpText<"Add directory to the system import search path">;
+
 def import_underlying_module : Flag<["-"], "import-underlying-module">,
   Flags<[FrontendOption, NoInteractiveOption]>,
   HelpText<"Implicitly imports the Objective-C half of a module">;

lib/AST/ASTContext.cpp

@@ -824,8 +824,8 @@ ASTContext::ASTContext(
 #include "swift/AST/KnownIdentifiers.def"
 
   // Record the initial set of search paths.
-  for (StringRef path : SearchPathOpts.getImportSearchPaths())
-    getImpl().SearchPathsSet[path] |= SearchPathKind::Import;
+  for (const auto &path : SearchPathOpts.getImportSearchPaths())
+    getImpl().SearchPathsSet[path.Path] |= SearchPathKind::Import;
   for (const auto &framepath : SearchPathOpts.getFrameworkSearchPaths())
     getImpl().SearchPathsSet[framepath.Path] |= SearchPathKind::Framework;
 
@@ -2098,7 +2098,7 @@ void ASTContext::addSearchPath(StringRef searchPath, bool isFramework,
     SearchPathOpts.addFrameworkSearchPath({searchPath, isSystem},
                                           SourceMgr.getFileSystem().get());
   } else {
-    SearchPathOpts.addImportSearchPath(searchPath,
+    SearchPathOpts.addImportSearchPath({searchPath, isSystem},
                                        SourceMgr.getFileSystem().get());
   }
 
@@ -2168,8 +2168,8 @@ Identifier ASTContext::getRealModuleName(Identifier key, ModuleAliasLookupOption
 }
 
 namespace {
-  static StringRef
-  pathStringFromFrameworkSearchPath(const SearchPathOptions::FrameworkSearchPath &next) {
+  static StringRef pathStringFromSearchPath(
+      const SearchPathOptions::SearchPath &next) {
     return next.Path;
   }
 }
@@ -2183,16 +2183,16 @@ const {
 llvm::StringSet<> ASTContext::getAllModuleSearchPathsSet()
 const {
   llvm::StringSet<> result;
-  auto ImportSearchPaths = SearchPathOpts.getImportSearchPaths();
-  result.insert(ImportSearchPaths.begin(), ImportSearchPaths.end());
-
-  // Framework paths are "special", they contain more than path strings,
-  // but path strings are all we care about here.
-  using FrameworkPathView = ArrayRefView<SearchPathOptions::FrameworkSearchPath,
-                                         StringRef,
-                                         pathStringFromFrameworkSearchPath>;
-  FrameworkPathView frameworkPathsOnly{
-      SearchPathOpts.getFrameworkSearchPaths()};
+
+  // Import and framework paths are "special", they contain more than path
+  // strings, but path strings are all we care about here.
+  using SearchPathView = ArrayRefView<SearchPathOptions::SearchPath, StringRef,
+                                      pathStringFromSearchPath>;
+
+  SearchPathView importPathsOnly{SearchPathOpts.getImportSearchPaths()};
+  result.insert(importPathsOnly.begin(), importPathsOnly.end());
+
+  SearchPathView frameworkPathsOnly{SearchPathOpts.getFrameworkSearchPaths()};
   result.insert(frameworkPathsOnly.begin(), frameworkPathsOnly.end());
 
   if (LangOpts.Target.isOSDarwin()) {

lib/AST/SearchPathOptions.cpp

@@ -48,9 +48,9 @@ void ModuleSearchPathLookup::rebuildLookupTable(const SearchPathOptions *Opts,
   clearLookupTable();
 
   for (auto Entry : llvm::enumerate(Opts->getImportSearchPaths())) {
-    addFilesInPathToLookupTable(FS, Entry.value(),
+    addFilesInPathToLookupTable(FS, Entry.value().Path,
                                 ModuleSearchPathKind::Import,
-                                /*isSystem=*/false, Entry.index());
+                                Entry.value().IsSystem, Entry.index());
   }
 
   for (auto Entry : llvm::enumerate(Opts->getFrameworkSearchPaths())) {
@@ -109,9 +109,11 @@ SearchPathOptions::getSDKPlatformPath(llvm::vfs::FileSystem *FS) const {
 }
 
 void SearchPathOptions::dump(bool isDarwin) const {
-  llvm::errs() << "Module import search paths (non system):\n";
+  llvm::errs() << "Module import search paths:\n";
   for (auto Entry : llvm::enumerate(getImportSearchPaths())) {
-    llvm::errs() << "  [" << Entry.index() << "] " << Entry.value() << "\n";
+    llvm::errs() << "  [" << Entry.index() << "] "
+                 << (Entry.value().IsSystem ? "(system) " : "(non-system) ")
+                 << Entry.value().Path << "\n";
   }
 
   llvm::errs() << "Framework search paths:\n";

lib/ClangImporter/ClangImporter.cpp

@@ -206,20 +206,6 @@ namespace {
       return std::make_unique<HeaderParsingASTConsumer>(Impl);
     }
     bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-      // Prefer frameworks over plain headers.
-      // We add search paths here instead of when building the initial invocation
-      // so that (a) we use the same code as search paths for imported modules,
-      // and (b) search paths are always added after -Xcc options.
-      SearchPathOptions &searchPathOpts = Ctx.SearchPathOpts;
-      for (const auto &framepath : searchPathOpts.getFrameworkSearchPaths()) {
-        Importer.addSearchPath(framepath.Path, /*isFramework*/true,
-                               framepath.IsSystem);
-      }
-
-      for (const auto &path : searchPathOpts.getImportSearchPaths()) {
-        Importer.addSearchPath(path, /*isFramework*/false, /*isSystem=*/false);
-      }
-
       auto PCH =
           Importer.getOrCreatePCH(ImporterOpts, SwiftPCHHash, /*Cached=*/true);
       if (PCH.has_value()) {
@@ -890,19 +876,19 @@ importer::addCommonInvocationArguments(
     }
   }
 
-  if (std::optional<StringRef> R = ctx.SearchPathOpts.getWinSDKRoot()) {
+  if (std::optional<StringRef> R = searchPathOpts.getWinSDKRoot()) {
     invocationArgStrs.emplace_back("-Xmicrosoft-windows-sdk-root");
     invocationArgStrs.emplace_back(*R);
   }
-  if (std::optional<StringRef> V = ctx.SearchPathOpts.getWinSDKVersion()) {
+  if (std::optional<StringRef> V = searchPathOpts.getWinSDKVersion()) {
     invocationArgStrs.emplace_back("-Xmicrosoft-windows-sdk-version");
     invocationArgStrs.emplace_back(*V);
   }
-  if (std::optional<StringRef> R = ctx.SearchPathOpts.getVCToolsRoot()) {
+  if (std::optional<StringRef> R = searchPathOpts.getVCToolsRoot()) {
     invocationArgStrs.emplace_back("-Xmicrosoft-visualc-tools-root");
     invocationArgStrs.emplace_back(*R);
   }
-  if (std::optional<StringRef> V = ctx.SearchPathOpts.getVCToolsVersion()) {
+  if (std::optional<StringRef> V = searchPathOpts.getVCToolsVersion()) {
     invocationArgStrs.emplace_back("-Xmicrosoft-visualc-tools-version");
     invocationArgStrs.emplace_back(*V);
   }
@@ -948,6 +934,24 @@ importer::addCommonInvocationArguments(
   for (auto extraArg : importerOpts.ExtraArgs) {
     invocationArgStrs.push_back(extraArg);
   }
+
+  for (const auto &framepath : searchPathOpts.getFrameworkSearchPaths()) {
+    if (framepath.IsSystem) {
+      invocationArgStrs.push_back("-iframework");
+      invocationArgStrs.push_back(framepath.Path);
+    } else {
+      invocationArgStrs.push_back("-F" + framepath.Path);
+    }
+  }
+
+  for (const auto &path : searchPathOpts.getImportSearchPaths()) {
+    if (path.IsSystem) {
+      invocationArgStrs.push_back("-isystem");
+      invocationArgStrs.push_back(path.Path);
+    } else {
+      invocationArgStrs.push_back("-I" + path.Path);
+    }
+  }
 }
 
 bool ClangImporter::canReadPCH(StringRef PCHFilename) {

lib/ClangImporter/ClangModuleDependencyScanner.cpp

@@ -61,24 +61,9 @@ moduleCacheRelativeLookupModuleOutput(const ModuleID &MID, ModuleOutputKind MOK,
   return outputPath.str().str();
 }
 
-// Add search paths.
-// Note: This is handled differently for the Clang importer itself, which
-// adds search paths to Clang's data structures rather than to its
-// command line.
-static void addSearchPathInvocationArguments(
+static void addScannerPrefixMapperInvocationArguments(
     std::vector<std::string> &invocationArgStrs, ASTContext &ctx) {
-  SearchPathOptions &searchPathOpts = ctx.SearchPathOpts;
-  for (const auto &framepath : searchPathOpts.getFrameworkSearchPaths()) {
-    invocationArgStrs.push_back(framepath.IsSystem ? "-iframework" : "-F");
-    invocationArgStrs.push_back(framepath.Path);
-  }
-
-  for (const auto &path : searchPathOpts.getImportSearchPaths()) {
-    invocationArgStrs.push_back("-I");
-    invocationArgStrs.push_back(path);
-  }
-
-  for (const auto &arg : searchPathOpts.ScannerPrefixMapper) {
+  for (const auto &arg : ctx.SearchPathOpts.ScannerPrefixMapper) {
     std::string prefixMapArg = "-fdepscan-prefix-map=" + arg;
     invocationArgStrs.push_back(prefixMapArg);
   }
@@ -89,7 +74,7 @@ static std::vector<std::string> getClangDepScanningInvocationArguments(
     ASTContext &ctx, std::optional<StringRef> sourceFileName = std::nullopt) {
   std::vector<std::string> commandLineArgs =
       ClangImporter::getClangDriverArguments(ctx);
-  addSearchPathInvocationArguments(commandLineArgs, ctx);
+  addScannerPrefixMapperInvocationArguments(commandLineArgs, ctx);
 
   auto sourceFilePos = std::find(
       commandLineArgs.begin(), commandLineArgs.end(),

lib/DriverTool/sil_func_extractor_main.cpp

@@ -242,7 +242,11 @@ int sil_func_extractor_main(ArrayRef<const char *> argv, void *MainAddr) {
   Invocation.setMainExecutablePath(llvm::sys::fs::getMainExecutable(argv[0], MainAddr));
 
   // Give the context the list of search paths to use for modules.
-  Invocation.setImportSearchPaths(options.ImportPaths);
+  std::vector<SearchPathOptions::SearchPath> ImportPaths;
+  for (const auto &path : options.ImportPaths) {
+    ImportPaths.push_back({path, /*isSystem=*/false});
+  }
+  Invocation.setImportSearchPaths(ImportPaths);
   // Set the SDK path and target if given.
   if (options.SDKPath.getNumOccurrences() == 0) {
     const char *SDKROOT = getenv("SDKROOT");

lib/DriverTool/sil_llvm_gen_main.cpp

@@ -129,8 +129,12 @@ int sil_llvm_gen_main(ArrayRef<const char *> argv, void *MainAddr) {
   Invocation.setMainExecutablePath(llvm::sys::fs::getMainExecutable(argv[0], MainAddr));
 
   // Give the context the list of search paths to use for modules.
-  Invocation.setImportSearchPaths(options.ImportPaths);
-  std::vector<SearchPathOptions::FrameworkSearchPath> FramePaths;
+  std::vector<SearchPathOptions::SearchPath> ImportPaths;
+  for (const auto &path : options.ImportPaths) {
+    ImportPaths.push_back({path, /*isSystem=*/false});
+  }
+  Invocation.setImportSearchPaths(ImportPaths);
+  std::vector<SearchPathOptions::SearchPath> FramePaths;
   for (const auto &path : options.FrameworkPaths) {
     FramePaths.push_back({path, /*isSystem=*/false});
   }