[clang] 8f0dd4e - [clang] Canonicalize system headers in dependency file when -canonical-prefixes
Arthur Eubanks via cfe-commits
cfe-commits at lists.llvm.org
Mon May 1 10:47:17 PDT 2023
Author: Arthur Eubanks
Date: 2023-05-01T10:40:18-07:00
New Revision: 8f0dd4ef3ed2137d1e2554204127434fc46ee190
URL: https://github.com/llvm/llvm-project/commit/8f0dd4ef3ed2137d1e2554204127434fc46ee190
DIFF: https://github.com/llvm/llvm-project/commit/8f0dd4ef3ed2137d1e2554204127434fc46ee190.diff
LOG: [clang] Canonicalize system headers in dependency file when -canonical-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 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.
Reviewed By: hans
Differential Revision: https://reviews.llvm.org/D149187
Added:
clang/test/Driver/canonical-system-headers.c
clang/test/Preprocessor/Inputs/canonical-system-headers/a.h
clang/test/Preprocessor/canonical-system-headers.c
Modified:
clang/include/clang/Driver/Options.td
clang/include/clang/Frontend/DependencyOutputOptions.h
clang/include/clang/Frontend/Utils.h
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/DependencyFile.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 8c80c7f5ff56a..182f0290736d8 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/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">>;
diff --git a/clang/include/clang/Frontend/DependencyOutputOptions.h b/clang/include/clang/Frontend/DependencyOutputOptions.h
index e0f445bb5970c..e140ff9baa117 100644
--- a/clang/include/clang/Frontend/DependencyOutputOptions.h
+++ b/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
diff --git a/clang/include/clang/Frontend/Utils.h b/clang/include/clang/Frontend/Utils.h
index 143cf4359f00b..8300e45d15fe5 100644
--- a/clang/include/clang/Frontend/Utils.h
+++ b/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;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 33ad50ad13483..5b93b84dfcf15 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/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))
diff --git a/clang/lib/Frontend/DependencyFile.cpp b/clang/lib/Frontend/DependencyFile.cpp
index fe4218b6e672d..44268e71dc241 100644
--- a/clang/lib/Frontend/DependencyFile.cpp
+++ b/clang/lib/Frontend/DependencyFile.cpp
@@ -49,6 +49,7 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
DepCollector.maybeAddDependency(
llvm::sys::path::remove_leading_dotslash(*Filename),
/*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
+ &PP.getFileManager(),
/*IsMissing*/ false);
}
@@ -56,9 +57,11 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
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),
diff --git a/clang/test/Driver/canonical-system-headers.c b/clang/test/Driver/canonical-system-headers.c
new file mode 100644
index 0000000000000..a7ab57521fc22
--- /dev/null
+++ b/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"
diff --git a/clang/test/Preprocessor/Inputs/canonical-system-headers/a.h b/clang/test/Preprocessor/Inputs/canonical-system-headers/a.h
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Preprocessor/canonical-system-headers.c b/clang/test/Preprocessor/canonical-system-headers.c
new file mode 100644
index 0000000000000..ec443d11838d5
--- /dev/null
+++ b/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>
More information about the cfe-commits
mailing list