[clang] Canonicalize system headers in dependency file when -canonical-prefixes

[aeubanks]

Summary

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 https://github.com/ninja-build/ninja/issues/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.

[hans]

Thanks! I like it.

clang/include/clang/Frontend/Utils.h

44

nit: sort?

[chapuni]

I am afraid this would be incompatible to bazel due to deep symlinks for sandbox.

[aeubanks]

In D149187#4311354, @chapuni wrote:

I am afraid this would be incompatible to bazel due to deep symlinks for sandbox.

I assume bazel would use -no-canonical-prefixes?

[shafik]

This commit came up in the following bug report: https://github.com/llvm/llvm-project/issues/62505

Please take a look.

clang/include/clang/Driver/Options.td

 // Dependency Output Options
 //===----------------------------------------------------------------------===//
 
 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">>;
 def header_include_file : Separate<["-"], "header-include-file">,
   HelpText<"Filename (or -) to write header include output to">,
   MarshallingInfoString<DependencyOutputOpts<"HeaderIncludeOutputFile">>;
 def header_include_format_EQ : Joined<["-"], "header-include-format=">,
   HelpText<"set format in which header info is emitted">,

clang/include/clang/Frontend/DependencyOutputOptions.h

enum ExtraDepKind {
   EDK_DepFileEntry,
 };
 
 /// DependencyOutputOptions - Options for controlling the compiler dependency
 /// file generation.
 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'
                                      /// problems.
   unsigned AddMissingHeaderDeps : 1; ///< Add missing headers to dependency list
   unsigned IncludeModuleFiles : 1; ///< Include module file dependencies.
   unsigned ShowSkippedHeaderIncludes : 1; ///< With ShowHeaderIncludes, show
                                           /// also includes that were skipped
public:
   /// The file to write GraphViz-formatted header dependencies to.
   std::string DOTOutputFile;
 
   /// The directory to copy module dependencies to when collecting them.
   std::string ModuleDependencyOutputDir;
 
 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
 
 #endif

clang/include/clang/Frontend/Utils.h

 class ASTReader;
 class CompilerInstance;
 class CompilerInvocation;
 class DiagnosticsEngine;
 class ExternalSemaSource;
 class FrontendOptions;
 class PCHContainerReader;
 class Preprocessor;
+class FileManager;
 class PreprocessorOptions;
 class PreprocessorOutputOptions;
 
 /// InitializePreprocessor - Initialize the preprocessor getting it and the
 /// environment ready to process a single file.
 void InitializePreprocessor(Preprocessor &PP, const PreprocessorOptions &PPOpts,
                             const PCHContainerReader &PCHContainerRdr,
                             const FrontendOptions &FEOpts);
virtual bool sawDependency(StringRef Filename, bool FromModule,
                              bool IsSystem, bool IsModuleFile, bool IsMissing);
 
   /// Called when the end of the main file is reached.
   virtual void finishedMainFile(DiagnosticsEngine &Diags) {}
 
   /// 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
   /// otherwise.
   bool addDependency(StringRef Filename);
 
 private:
   llvm::StringSet<> Seen;
public:
 
   void finishedMainFile(DiagnosticsEngine &Diags) override;
 
   bool needSystemDependencies() final { return IncludeSystemHeaders; }
 
   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);
 
 private:
   void outputDependencyFile(DiagnosticsEngine &Diags);
 
   std::string OutputFile;
   std::vector<std::string> Targets;
   bool IncludeSystemHeaders;
+  bool CanonicalSystemHeaders;
   bool PhonyTarget;
   bool AddMissingHeaderDeps;
   bool SeenMissingHeader;
   bool IncludeModuleFiles;
   DependencyOutputFormat OutputFormat;
   unsigned InputFileIndex;
 };
 

clang/lib/Driver/ToolChains/Clang.cpp

if (!HasTarget) {
       SmallString<128> Quoted;
       quoteMakeTarget(DepTarget, Quoted);
       CmdArgs.push_back(Args.MakeArgString(Quoted));
     }
 
     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))
       CmdArgs.push_back("-module-file-deps");
   }
 
   if (Args.hasArg(options::OPT_MG)) {
     if (!ArgM || ArgM->getOption().matches(options::OPT_MD) ||

clang/lib/Frontend/DependencyFile.cpp

void LexedFileChanged(FileID FID, LexedFileChangeReason Reason,
     // Dependency generation really does want to go all the way to the
     // file entry for a source location to find out what is depended on.
     // We do not want #line markers to affect dependency generation!
     if (std::optional<StringRef> Filename =
             PP.getSourceManager().getNonBuiltinFilenameForID(FID))
       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);
   }
 
   void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
                           StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange,
                           OptionalFileEntryRef File, StringRef SearchPath,
                           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.
   }
 
   void HasInclude(SourceLocation Loc, StringRef SpelledFilename, bool IsAngled,
                   OptionalFileEntryRef File,
                   SrcMgr::CharacteristicKind FileType) override {
     if (!File)
       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);
   }
 
   void EndOfMainFile() override {
     DepCollector.finishedMainFile(PP.getDiagnostics());
   }
 };
 
 struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
   DependencyCollector &DepCollector;
   DepCollectorMMCallbacks(DependencyCollector &DC) : DepCollector(DC) {}
 
   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);
   }
 };
 
 struct DepCollectorASTListener : public ASTReaderListener {
   DependencyCollector &DepCollector;
   FileManager &FileMgr;
   DepCollectorASTListener(DependencyCollector &L, FileManager &FileMgr)
       : DepCollector(L), FileMgr(FileMgr) {}
   bool needsInputFileVisitation() override { return true; }
   bool needsSystemInputFileVisitation() override {
     return DepCollector.needSystemDependencies();
   }
   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 {
     if (IsOverridden || IsExplicitModule)
       return true;
 
     // Run this through the FileManager in order to respect 'use-external-name'
     // in case we have a VFS overlay.
     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;
   }
 };
 } // end anonymous namespace
 
 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) {
   StringRef SearchPath;
 #ifdef _WIN32
   // Make the search insensitive to case and separators.
   llvm::SmallString<256> TmpPath = Filename;
   llvm::sys::path::native(TmpPath);
   std::transform(TmpPath.begin(), TmpPath.end(), TmpPath.begin(), ::tolower);
void DependencyCollector::attachToASTReader(ASTReader &R) {
   R.addListener(
       std::make_unique<DepCollectorASTListener>(*this, R.getFileManager()));
 }
 
 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),
       OutputFormat(Opts.OutputFormat), InputFileIndex(0) {
   for (const auto &ExtraDep : Opts.ExtraDeps) {
     if (addDependency(ExtraDep.first))
       ++InputFileIndex;
   }

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

+// 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

+// 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>