Skip to content

Commit 578a471

Browse files
aeubanksaeubanks
committed
Reland [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 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
1 parent 17a8e24 commit 578a471

File tree

8 files changed

+80
-20
lines changed

8 files changed

+80
-20
lines changed

‎clang/include/clang/Driver/Options.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5835,6 +5835,9 @@ let Flags = [CC1Option, NoDriverOption] in {
58355835
def sys_header_deps : Flag<["-"], "sys-header-deps">,
58365836
HelpText<"Include system headers in dependency output">,
58375837
MarshallingInfoFlag<DependencyOutputOpts<"IncludeSystemHeaders">>;
5838+
def canonical_system_headers : Flag<["-"], "canonical-system-headers">,
5839+
HelpText<"Canonicalize system headers in dependency output">,
5840+
MarshallingInfoFlag<DependencyOutputOpts<"CanonicalSystemHeaders">>;
58385841
def module_file_deps : Flag<["-"], "module-file-deps">,
58395842
HelpText<"Include module files in dependency output">,
58405843
MarshallingInfoFlag<DependencyOutputOpts<"IncludeModuleFiles">>;

‎clang/include/clang/Frontend/DependencyOutputOptions.h

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ enum ExtraDepKind {
3434
class DependencyOutputOptions {
3535
public:
3636
unsigned IncludeSystemHeaders : 1; ///< Include system header dependencies.
37+
unsigned
38+
CanonicalSystemHeaders : 1; ///< canonicalize system header dependencies.
3739
unsigned ShowHeaderIncludes : 1; ///< Show header inclusions (-H).
3840
unsigned UsePhonyTargets : 1; ///< Include phony targets for each
3941
/// dependency, which can avoid some 'make'
@@ -85,10 +87,11 @@ class DependencyOutputOptions {
8587

8688
public:
8789
DependencyOutputOptions()
88-
: IncludeSystemHeaders(0), ShowHeaderIncludes(0), UsePhonyTargets(0),
89-
AddMissingHeaderDeps(0), IncludeModuleFiles(0),
90-
ShowSkippedHeaderIncludes(0), HeaderIncludeFormat(HIFMT_Textual),
91-
HeaderIncludeFiltering(HIFIL_None) {}
90+
: IncludeSystemHeaders(0), CanonicalSystemHeaders(0),
91+
ShowHeaderIncludes(0), UsePhonyTargets(0), AddMissingHeaderDeps(0),
92+
IncludeModuleFiles(0), ShowSkippedHeaderIncludes(0),
93+
HeaderIncludeFormat(HIFMT_Textual), HeaderIncludeFiltering(HIFIL_None) {
94+
}
9295
};
9396

9497
} // end namespace clang

‎clang/include/clang/Frontend/Utils.h

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ class ExternalSemaSource;
4141
class FrontendOptions;
4242
class PCHContainerReader;
4343
class Preprocessor;
44+
class FileManager;
4445
class PreprocessorOptions;
4546
class PreprocessorOutputOptions;
4647

@@ -79,11 +80,14 @@ class DependencyCollector {
7980
/// Return true if system files should be passed to sawDependency().
8081
virtual bool needSystemDependencies() { return false; }
8182

83+
/// Return true if system files should be canonicalized.
84+
virtual bool shouldCanonicalizeSystemDependencies() { return false; }
85+
8286
/// Add a dependency \p Filename if it has not been seen before and
8387
/// sawDependency() returns true.
8488
virtual void maybeAddDependency(StringRef Filename, bool FromModule,
8589
bool IsSystem, bool IsModuleFile,
86-
bool IsMissing);
90+
FileManager *FileMgr, bool IsMissing);
8791

8892
protected:
8993
/// Return true if the filename was added to the list of dependencies, false
@@ -112,6 +116,10 @@ class DependencyFileGenerator : public DependencyCollector {
112116
bool sawDependency(StringRef Filename, bool FromModule, bool IsSystem,
113117
bool IsModuleFile, bool IsMissing) final;
114118

119+
bool shouldCanonicalizeSystemDependencies() override {
120+
return CanonicalSystemHeaders;
121+
}
122+
115123
protected:
116124
void outputDependencyFile(llvm::raw_ostream &OS);
117125

@@ -121,6 +129,7 @@ class DependencyFileGenerator : public DependencyCollector {
121129
std::string OutputFile;
122130
std::vector<std::string> Targets;
123131
bool IncludeSystemHeaders;
132+
bool CanonicalSystemHeaders;
124133
bool PhonyTarget;
125134
bool AddMissingHeaderDeps;
126135
bool SeenMissingHeader;

‎clang/lib/Driver/ToolChains/Clang.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1139,6 +1139,9 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
11391139
if (ArgM->getOption().matches(options::OPT_M) ||
11401140
ArgM->getOption().matches(options::OPT_MD))
11411141
CmdArgs.push_back("-sys-header-deps");
1142+
if (Args.hasFlag(options::OPT_canonical_prefixes,
1143+
options::OPT_no_canonical_prefixes, true))
1144+
CmdArgs.push_back("-canonical-system-headers");
11421145
if ((isa<PrecompileJobAction>(JA) &&
11431146
!Args.hasArg(options::OPT_fno_module_file_deps)) ||
11441147
Args.hasArg(options::OPT_fmodule_file_deps))

‎clang/lib/Frontend/DependencyFile.cpp

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,19 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
4949
DepCollector.maybeAddDependency(
5050
llvm::sys::path::remove_leading_dotslash(*Filename),
5151
/*FromModule*/ false, isSystem(FileType), /*IsModuleFile*/ false,
52+
&PP.getFileManager(),
5253
/*IsMissing*/ false);
5354
}
5455

5556
void FileSkipped(const FileEntryRef &SkippedFile, const Token &FilenameTok,
5657
SrcMgr::CharacteristicKind FileType) override {
5758
StringRef Filename =
5859
llvm::sys::path::remove_leading_dotslash(SkippedFile.getName());
59-
DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
60+
DepCollector.maybeAddDependency(Filename,
61+
/*FromModule=*/false,
6062
/*IsSystem=*/isSystem(FileType),
6163
/*IsModuleFile=*/false,
64+
&PP.getFileManager(),
6265
/*IsMissing=*/false);
6366
}
6467

@@ -69,9 +72,12 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
6972
StringRef RelativePath, const Module *Imported,
7073
SrcMgr::CharacteristicKind FileType) override {
7174
if (!File)
72-
DepCollector.maybeAddDependency(FileName, /*FromModule*/false,
73-
/*IsSystem*/false, /*IsModuleFile*/false,
74-
/*IsMissing*/true);
75+
DepCollector.maybeAddDependency(FileName,
76+
/*FromModule*/ false,
77+
/*IsSystem*/ false,
78+
/*IsModuleFile*/ false,
79+
&PP.getFileManager(),
80+
/*IsMissing*/ true);
7581
// Files that actually exist are handled by FileChanged.
7682
}
7783

@@ -82,9 +88,11 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
8288
return;
8389
StringRef Filename =
8490
llvm::sys::path::remove_leading_dotslash(File->getName());
85-
DepCollector.maybeAddDependency(Filename, /*FromModule=*/false,
91+
DepCollector.maybeAddDependency(Filename,
92+
/*FromModule=*/false,
8693
/*IsSystem=*/isSystem(FileType),
8794
/*IsModuleFile=*/false,
95+
&PP.getFileManager(),
8896
/*IsMissing=*/false);
8997
}
9098

@@ -100,10 +108,12 @@ struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
100108
void moduleMapFileRead(SourceLocation Loc, const FileEntry &Entry,
101109
bool IsSystem) override {
102110
StringRef Filename = Entry.getName();
103-
DepCollector.maybeAddDependency(Filename, /*FromModule*/false,
104-
/*IsSystem*/IsSystem,
105-
/*IsModuleFile*/false,
106-
/*IsMissing*/false);
111+
DepCollector.maybeAddDependency(Filename,
112+
/*FromModule*/ false,
113+
/*IsSystem*/ IsSystem,
114+
/*IsModuleFile*/ false,
115+
/*FileMgr*/ nullptr,
116+
/*IsMissing*/ false);
107117
}
108118
};
109119

@@ -118,9 +128,11 @@ struct DepCollectorASTListener : public ASTReaderListener {
118128
}
119129
void visitModuleFile(StringRef Filename,
120130
serialization::ModuleKind Kind) override {
121-
DepCollector.maybeAddDependency(Filename, /*FromModule*/true,
122-
/*IsSystem*/false, /*IsModuleFile*/true,
123-
/*IsMissing*/false);
131+
DepCollector.maybeAddDependency(Filename,
132+
/*FromModule*/ true,
133+
/*IsSystem*/ false, /*IsModuleFile*/ true,
134+
/*FileMgr*/ nullptr,
135+
/*IsMissing*/ false);
124136
}
125137
bool visitInputFile(StringRef Filename, bool IsSystem,
126138
bool IsOverridden, bool IsExplicitModule) override {
@@ -132,8 +144,9 @@ struct DepCollectorASTListener : public ASTReaderListener {
132144
if (auto FE = FileMgr.getOptionalFileRef(Filename))
133145
Filename = FE->getName();
134146

135-
DepCollector.maybeAddDependency(Filename, /*FromModule*/true, IsSystem,
136-
/*IsModuleFile*/false, /*IsMissing*/false);
147+
DepCollector.maybeAddDependency(Filename, /*FromModule*/ true, IsSystem,
148+
/*IsModuleFile*/ false, /*FileMgr*/ nullptr,
149+
/*IsMissing*/ false);
137150
return true;
138151
}
139152
};
@@ -142,9 +155,15 @@ struct DepCollectorASTListener : public ASTReaderListener {
142155
void DependencyCollector::maybeAddDependency(StringRef Filename,
143156
bool FromModule, bool IsSystem,
144157
bool IsModuleFile,
158+
FileManager *FileMgr,
145159
bool IsMissing) {
146-
if (sawDependency(Filename, FromModule, IsSystem, IsModuleFile, IsMissing))
160+
if (sawDependency(Filename, FromModule, IsSystem, IsModuleFile, IsMissing)) {
161+
if (IsSystem && FileMgr && shouldCanonicalizeSystemDependencies()) {
162+
if (auto F = FileMgr->getFile(Filename))
163+
Filename = FileMgr->getCanonicalName(*F);
164+
}
147165
addDependency(Filename);
166+
}
148167
}
149168

150169
bool DependencyCollector::addDependency(StringRef Filename) {
@@ -192,6 +211,7 @@ DependencyFileGenerator::DependencyFileGenerator(
192211
const DependencyOutputOptions &Opts)
193212
: OutputFile(Opts.OutputFile), Targets(Opts.Targets),
194213
IncludeSystemHeaders(Opts.IncludeSystemHeaders),
214+
CanonicalSystemHeaders(Opts.CanonicalSystemHeaders),
195215
PhonyTarget(Opts.UsePhonyTargets),
196216
AddMissingHeaderDeps(Opts.AddMissingHeaderDeps), SeenMissingHeader(false),
197217
IncludeModuleFiles(Opts.IncludeModuleFiles),

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// RUN: %clang -MD -no-canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO
2+
// RUN: %clang -MD -canonical-prefixes -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
3+
// RUN: %clang -MD -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-YES
4+
5+
// CHECK-YES: "-canonical-system-headers"
6+
// CHECK-NO-NOT: "-canonical-system-headers"

‎clang/test/Preprocessor/Inputs/canonical-system-headers/a.h

Whitespace-only changes.

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// don't create symlinks on windows
2+
// UNSUPPORTED: system-windows
3+
// REQUIRES: shell
4+
5+
// RUN: rm -rf %t
6+
// RUN: mkdir -p %t/foo/
7+
// RUN: ln -f -s %S/Inputs/canonical-system-headers %t/foo/include
8+
// RUN: %clang_cc1 -isystem %t/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only
9+
// RUN: FileCheck %s --check-prefix=NOCANON --implicit-check-not=a.h < %t2
10+
// RUN: %clang_cc1 -isystem %t/foo/include -sys-header-deps -MT foo.o -dependency-file %t2 %s -fsyntax-only -canonical-system-headers
11+
// RUN: FileCheck %s --check-prefix=CANON --implicit-check-not=a.h < %t2
12+
13+
// NOCANON: foo/include/a.h
14+
// CANON: Inputs/canonical-system-headers/a.h
15+
16+
#include <a.h>

0 commit comments

Comments
 (0)