Reland [clang] Canonicalize system headers in dependency file when -c…

…anonical-prefixes

Clang was writing paths to the dependency file that don't exist when using a sysroot with symlinks, causing everything to get rebuilt every time. This is reproducible on Linux by creating a symlink to '/', using that as the sysroot, and trying to build something with ninja that includes the C++ stdlib (e.g. a typical build of LLVM).

This fixes ninja-build/ninja#1330 and somewhat matches gcc.

gcc canonicalizes system headers in dependency files under a -f[no-]canonical-system-headers, but it makes more sense to look at -canonical-prefixes.

D37954 was a previous attempt at this.

Fixed use of %T instead of %t in test, causing bots to fail the test on the initial commit.

Reviewed By: hans

Differential Revision: https://reviews.llvm.org/D149187

clang/include/clang/Driver/Options.td

@@ -5835,6 +5835,9 @@ let Flags = [CC1Option, NoDriverOption] in {
 def sys_header_deps : Flag<["-"], "sys-header-deps">,
   HelpText<"Include system headers in dependency output">,
   MarshallingInfoFlag<DependencyOutputOpts<"IncludeSystemHeaders">>;
+def canonical_system_headers : Flag<["-"], "canonical-system-headers">,
+  HelpText<"Canonicalize system headers in dependency output">,
+  MarshallingInfoFlag<DependencyOutputOpts<"CanonicalSystemHeaders">>;
 def module_file_deps : Flag<["-"], "module-file-deps">,
   HelpText<"Include module files in dependency output">,
   MarshallingInfoFlag<DependencyOutputOpts<"IncludeModuleFiles">>;

clang/include/clang/Frontend/DependencyOutputOptions.h

@@ -34,6 +34,8 @@ enum ExtraDepKind {
 class DependencyOutputOptions {
 public:
   unsigned IncludeSystemHeaders : 1; ///< Include system header dependencies.
+  unsigned
+      CanonicalSystemHeaders : 1; ///< canonicalize system header dependencies.
   unsigned ShowHeaderIncludes : 1;   ///< Show header inclusions (-H).
   unsigned UsePhonyTargets : 1;      ///< Include phony targets for each
                                      /// dependency, which can avoid some 'make'
@@ -85,10 +87,11 @@ class DependencyOutputOptions {
 
 public:
   DependencyOutputOptions()
-      : IncludeSystemHeaders(0), ShowHeaderIncludes(0), UsePhonyTargets(0),
-        AddMissingHeaderDeps(0), IncludeModuleFiles(0),
-        ShowSkippedHeaderIncludes(0), HeaderIncludeFormat(HIFMT_Textual),
-        HeaderIncludeFiltering(HIFIL_None) {}
+      : IncludeSystemHeaders(0), CanonicalSystemHeaders(0),
+        ShowHeaderIncludes(0), UsePhonyTargets(0), AddMissingHeaderDeps(0),
+        IncludeModuleFiles(0), ShowSkippedHeaderIncludes(0),
+        HeaderIncludeFormat(HIFMT_Textual), HeaderIncludeFiltering(HIFIL_None) {
+  }
 };
 
 }  // end namespace clang

clang/include/clang/Frontend/Utils.h

@@ -41,6 +41,7 @@ class ExternalSemaSource;
 class FrontendOptions;
 class PCHContainerReader;
 class Preprocessor;
+class FileManager;
 class PreprocessorOptions;
 class PreprocessorOutputOptions;
 
@@ -79,11 +80,14 @@ class DependencyCollector {
   /// Return true if system files should be passed to sawDependency().
   virtual bool needSystemDependencies() { return false; }
 
+  /// Return true if system files should be canonicalized.
+  virtual bool shouldCanonicalizeSystemDependencies() { return false; }
+
   /// Add a dependency \p Filename if it has not been seen before and
   /// sawDependency() returns true.
   virtual void maybeAddDependency(StringRef Filename, bool FromModule,
                                   bool IsSystem, bool IsModuleFile,
-                                  bool IsMissing);
+                                  FileManager *FileMgr, bool IsMissing);
 
 protected:
   /// Return true if the filename was added to the list of dependencies, false
@@ -112,6 +116,10 @@ class DependencyFileGenerator : public DependencyCollector {
   bool sawDependency(StringRef Filename, bool FromModule, bool IsSystem,
                      bool IsModuleFile, bool IsMissing) final;
 
+  bool shouldCanonicalizeSystemDependencies() override {
+    return CanonicalSystemHeaders;
+  }
+
 protected:
   void outputDependencyFile(llvm::raw_ostream &OS);
 
@@ -121,6 +129,7 @@ class DependencyFileGenerator : public DependencyCollector {
   std::string OutputFile;
   std::vector<std::string> Targets;
   bool IncludeSystemHeaders;
+  bool CanonicalSystemHeaders;
   bool PhonyTarget;
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;

clang/lib/Driver/ToolChains/Clang.cpp

@@ -1139,6 +1139,9 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
     if (ArgM->getOption().matches(options::OPT_M) ||
         ArgM->getOption().matches(options::OPT_MD))
       CmdArgs.push_back("-sys-header-deps");
+    if (Args.hasFlag(options::OPT_canonical_prefixes,
+                     options::OPT_no_canonical_prefixes, true))
+      CmdArgs.push_back("-canonical-system-headers");
     if ((isa<PrecompileJobAction>(JA) &&
          !Args.hasArg(options::OPT_fno_module_file_deps)) ||
         Args.hasArg(options::OPT_fmodule_file_deps))

clang/lib/Frontend/DependencyFile.cpp

@@ -49,16 +49,19 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
       DepCollector.maybeAddDependency(
           llvm::sys::path::remove_leading_dotslash(*Filename),
           /*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
+          &PP.getFileManager(),
           /*IsMissing*/ false);
   }
 
   void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok,
                    SrcMgr::CharacteristicKind FileType) override {
     StringRef Filename =
         llvm::sys::path::remove_leading_dotslash(SkippedFile.getName());
-    DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+    DepCollector.maybeAddDependency(Filename,
+                                    /*FromModule=*/false,
                                     /*IsSystem=*/isSystem(FileType),
                                     /*IsModuleFile=*/false,
+                                    &PP.getFileManager(),
                                     /*IsMissing=*/false);
   }
 
@@ -69,9 +72,12 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
                           StringRef RelativePath, const Module *Imported,
                           SrcMgr::CharacteristicKind FileType) override {
     if (!File)
-      DepCollector.maybeAddDependency(FileName, /*FromModule*/false,
-                                     /*IsSystem*/false, /*IsModuleFile*/false,
-                                     /*IsMissing*/true);
+      DepCollector.maybeAddDependency(FileName,
+                                      /*FromModule*/ false,
+                                      /*IsSystem*/ false,
+                                      /*IsModuleFile*/ false,
+                                      &PP.getFileManager(),
+                                      /*IsMissing*/ true);
     // Files that actually exist are handled by FileChanged.
   }
 
@@ -82,9 +88,11 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
       return;
     StringRef Filename =
         llvm::sys::path::remove_leading_dotslash(File->getName());
-    DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
+    DepCollector.maybeAddDependency(Filename,
+                                    /*FromModule=*/false,
                                     /*IsSystem=*/isSystem(FileType),
                                     /*IsModuleFile=*/false,
+                                    &PP.getFileManager(),
                                     /*IsMissing=*/false);
   }
 
@@ -100,10 +108,12 @@ struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
   void moduleMapFileRead(SourceLocation Loc, const FileEntry &Entry,
                          bool IsSystem) override {
     StringRef Filename = Entry.getName();
-    DepCollector.maybeAddDependency(Filename, /*FromModule*/false,
-                                    /*IsSystem*/IsSystem,
-                                    /*IsModuleFile*/false,
-                                    /*IsMissing*/false);
+    DepCollector.maybeAddDependency(Filename,
+                                    /*FromModule*/ false,
+                                    /*IsSystem*/ IsSystem,
+                                    /*IsModuleFile*/ false,
+                                    /*FileMgr*/ nullptr,
+                                    /*IsMissing*/ false);
   }
 };
 
@@ -118,9 +128,11 @@ struct DepCollectorASTListener : public ASTReaderListener {
   }
   void visitModuleFile(StringRef Filename,
                        serialization::ModuleKind Kind) override {
-    DepCollector.maybeAddDependency(Filename, /*FromModule*/true,
-                                   /*IsSystem*/false, /*IsModuleFile*/true,
-                                   /*IsMissing*/false);
+    DepCollector.maybeAddDependency(Filename,
+                                    /*FromModule*/ true,
+                                    /*IsSystem*/ false, /*IsModuleFile*/ true,
+                                    /*FileMgr*/ nullptr,
+                                    /*IsMissing*/ false);
   }
   bool visitInputFile(StringRef Filename, bool IsSystem,
                       bool IsOverridden, bool IsExplicitModule) override {
@@ -132,8 +144,9 @@ struct DepCollectorASTListener : public ASTReaderListener {
     if (auto FE = FileMgr.getOptionalFileRef(Filename))
       Filename = FE->getName();
 
-    DepCollector.maybeAddDependency(Filename, /*FromModule*/true, IsSystem,
-                                   /*IsModuleFile*/false, /*IsMissing*/false);
+    DepCollector.maybeAddDependency(Filename, /*FromModule*/ true, IsSystem,
+                                    /*IsModuleFile*/ false, /*FileMgr*/ nullptr,
+                                    /*IsMissing*/ false);
     return true;
   }
 };
@@ -142,9 +155,15 @@ struct DepCollectorASTListener : public ASTReaderListener {
 void DependencyCollector::maybeAddDependency(StringRef Filename,
                                              bool FromModule, bool IsSystem,
                                              bool IsModuleFile,
+                                             FileManager *FileMgr,
                                              bool IsMissing) {
-  if (sawDependency(Filename, FromModule, IsSystem, IsModuleFile, IsMissing))
+  if (sawDependency(Filename, FromModule, IsSystem, IsModuleFile, IsMissing)) {
+    if (IsSystem && FileMgr && shouldCanonicalizeSystemDependencies()) {
+      if (auto F = FileMgr->getFile(Filename))
+        Filename = FileMgr->getCanonicalName(*F);
+    }
     addDependency(Filename);
+  }
 }
 
 bool DependencyCollector::addDependency(StringRef Filename) {
@@ -192,6 +211,7 @@ DependencyFileGenerator::DependencyFileGenerator(
     const DependencyOutputOptions &Opts)
     : OutputFile(Opts.OutputFile), Targets(Opts.Targets),
       IncludeSystemHeaders(Opts.IncludeSystemHeaders),
+      CanonicalSystemHeaders(Opts.CanonicalSystemHeaders),
       PhonyTarget(Opts.UsePhonyTargets),
       AddMissingHeaderDeps(Opts.AddMissingHeaderDeps), SeenMissingHeader(false),
       IncludeModuleFiles(Opts.IncludeModuleFiles),

clang/test/Driver/canonical-system-headers.c

@@ -0,0 +1,6 @@
+// RUN: %clang -MD -no-canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
+// RUN: %clang -MD -canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+// RUN: %clang -MD -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
+
+// CHECK-YES: "-canonical-system-headers"
+// CHECK-NO-NOT: "-canonical-system-headers"

clang/test/Preprocessor/canonical-system-headers.c

@@ -0,0 +1,16 @@
+// don't create symlinks on windows
+// UNSUPPORTED: system-windows
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/foo/
+// RUN: ln -f -s %S/Inputs/canonical-system-headers %t/foo/include
+// RUN: %clang_cc1 -isystem %t/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only
+// RUN: FileCheck %s --check-prefix=NOCANON --implicit-check-not=a.h < %t2
+// RUN: %clang_cc1 -isystem %t/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only -canonical-system-headers
+// RUN: FileCheck %s --check-prefix=CANON --implicit-check-not=a.h < %t2
+
+// NOCANON: foo/include/a.h
+// CANON: Inputs/canonical-system-headers/a.h
+
+#include <a.h>