Skip to content

Commit

Permalink
Revert "Reland [clang] Canonicalize system headers in dependency file…
Browse files Browse the repository at this point in the history
… when -canonical-prefixes" (#71697)

This reverts commit 578a471.

This causes multiple issues. Compile time slowdown due to more path
canonicalization, and weird behavior on Windows.

Will reland under a separate flag `-f[no-]canonical-system-headers` to
match gcc in the future and further limit when it's passed by default.

Fixes #70011.
  • Loading branch information
aeubanks committed Nov 14, 2023
1 parent 309d551 commit 3a4ed0a
Show file tree
Hide file tree
Showing 8 changed files with 12 additions and 70 deletions.
3 changes: 0 additions & 3 deletions clang/include/clang/Driver/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -6079,9 +6079,6 @@ 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">>;
Expand Down
11 changes: 4 additions & 7 deletions clang/include/clang/Frontend/DependencyOutputOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ 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'
Expand Down Expand Up @@ -84,11 +82,10 @@ class DependencyOutputOptions {

public:
DependencyOutputOptions()
: IncludeSystemHeaders(0), CanonicalSystemHeaders(0),
ShowHeaderIncludes(0), UsePhonyTargets(0), AddMissingHeaderDeps(0),
IncludeModuleFiles(0), ShowSkippedHeaderIncludes(0),
HeaderIncludeFormat(HIFMT_Textual), HeaderIncludeFiltering(HIFIL_None) {
}
: IncludeSystemHeaders(0), ShowHeaderIncludes(0), UsePhonyTargets(0),
AddMissingHeaderDeps(0), IncludeModuleFiles(0),
ShowSkippedHeaderIncludes(0), HeaderIncludeFormat(HIFMT_Textual),
HeaderIncludeFiltering(HIFIL_None) {}
};

} // end namespace clang
Expand Down
11 changes: 1 addition & 10 deletions clang/include/clang/Frontend/Utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ class ExternalSemaSource;
class FrontendOptions;
class PCHContainerReader;
class Preprocessor;
class FileManager;
class PreprocessorOptions;
class PreprocessorOutputOptions;

Expand Down Expand Up @@ -80,14 +79,11 @@ 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,
FileManager *FileMgr, bool IsMissing);
bool IsMissing);

protected:
/// Return true if the filename was added to the list of dependencies, false
Expand Down Expand Up @@ -116,10 +112,6 @@ 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);

Expand All @@ -129,7 +121,6 @@ class DependencyFileGenerator : public DependencyCollector {
std::string OutputFile;
std::vector<std::string> Targets;
bool IncludeSystemHeaders;
bool CanonicalSystemHeaders;
bool PhonyTarget;
bool AddMissingHeaderDeps;
bool SeenMissingHeader;
Expand Down
3 changes: 0 additions & 3 deletions clang/lib/Driver/ToolChains/Clang.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1152,9 +1152,6 @@ 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))
Expand Down
32 changes: 7 additions & 25 deletions clang/lib/Frontend/DependencyFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,16 @@ 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);
}

Expand All @@ -72,11 +69,9 @@ struct DepCollectorPPCallbacks : public PPCallbacks {
StringRef RelativePath, const Module *Imported,
SrcMgr::CharacteristicKind FileType) override {
if (!File)
DepCollector.maybeAddDependency(FileName,
/*FromModule*/ false,
DepCollector.maybeAddDependency(FileName, /*FromModule*/ false,
/*IsSystem*/ false,
/*IsModuleFile*/ false,
&PP.getFileManager(),
/*IsMissing*/ true);
// Files that actually exist are handled by FileChanged.
}
Expand All @@ -88,11 +83,9 @@ 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);
}

Expand All @@ -108,11 +101,9 @@ struct DepCollectorMMCallbacks : public ModuleMapCallbacks {
void moduleMapFileRead(SourceLocation Loc, const FileEntry &Entry,
bool IsSystem) override {
StringRef Filename = Entry.getName();
DepCollector.maybeAddDependency(Filename,
/*FromModule*/ false,
DepCollector.maybeAddDependency(Filename, /*FromModule*/ false,
/*IsSystem*/ IsSystem,
/*IsModuleFile*/ false,
/*FileMgr*/ nullptr,
/*IsMissing*/ false);
}
};
Expand All @@ -128,10 +119,8 @@ struct DepCollectorASTListener : public ASTReaderListener {
}
void visitModuleFile(StringRef Filename,
serialization::ModuleKind Kind) override {
DepCollector.maybeAddDependency(Filename,
/*FromModule*/ true,
DepCollector.maybeAddDependency(Filename, /*FromModule*/ true,
/*IsSystem*/ false, /*IsModuleFile*/ true,
/*FileMgr*/ nullptr,
/*IsMissing*/ false);
}
bool visitInputFile(StringRef Filename, bool IsSystem,
Expand All @@ -145,7 +134,7 @@ struct DepCollectorASTListener : public ASTReaderListener {
Filename = FE->getName();

DepCollector.maybeAddDependency(Filename, /*FromModule*/ true, IsSystem,
/*IsModuleFile*/ false, /*FileMgr*/ nullptr,
/*IsModuleFile*/ false,
/*IsMissing*/ false);
return true;
}
Expand All @@ -155,15 +144,9 @@ 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 (IsSystem && FileMgr && shouldCanonicalizeSystemDependencies()) {
if (auto F = FileMgr->getFile(Filename))
Filename = FileMgr->getCanonicalName(*F);
}
if (sawDependency(Filename, FromModule, IsSystem, IsModuleFile, IsMissing))
addDependency(Filename);
}
}

bool DependencyCollector::addDependency(StringRef Filename) {
Expand Down Expand Up @@ -211,7 +194,6 @@ 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),
Expand Down
6 changes: 0 additions & 6 deletions clang/test/Driver/canonical-system-headers.c

This file was deleted.

Empty file.
16 changes: 0 additions & 16 deletions clang/test/Preprocessor/canonical-system-headers.c

This file was deleted.

0 comments on commit 3a4ed0a

Please sign in to comment.