[clang] 5865476 - [clang][modules] Fix handling of `ModuleHeaderRole::ExcludedHeader`
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 6 16:20:33 PDT 2022
Author: Jan Svoboda
Date: 2022-10-06T16:20:24-07:00
New Revision: 586547687946b27a99d3bb4241226f9f126a173e
URL: https://github.com/llvm/llvm-project/commit/586547687946b27a99d3bb4241226f9f126a173e
DIFF: https://github.com/llvm/llvm-project/commit/586547687946b27a99d3bb4241226f9f126a173e.diff
LOG: [clang][modules] Fix handling of `ModuleHeaderRole::ExcludedHeader`
This is a follow-up to D134224. The original patch added new `ExcludedHeader` enumerator to `ModuleMap::ModuleHeaderRole` and started associating headers with the modules they were excluded from. This was necessary to consider their module maps as "affecting" in certain situations and in turn serialize them into the PCM.
The association of the header and module needs to be handled when deserializing the PCM as well, though. This patch fixes a potential assertion failure and a regression. This essentially reverts parts of feb54b6ded123f8118fdc20620d3f657dfeab485.
Reviewed By: Bigcheese
Differential Revision: https://reviews.llvm.org/D135381
Added:
clang/test/Modules/exclude-header-fw-umbrella.m
Modified:
clang/include/clang/Lex/ModuleMap.h
clang/lib/Lex/HeaderSearch.cpp
clang/lib/Lex/ModuleMap.cpp
clang/lib/Lex/PPDirectives.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h
index 10c5dfc25d9ce..ca04ce623616e 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -152,6 +152,9 @@ class ModuleMap {
/// Convert a header role to a kind.
static Module::HeaderKind headerRoleToKind(ModuleHeaderRole Role);
+ /// Check if the header with the given role is a modular one.
+ static bool isModular(ModuleHeaderRole Role);
+
/// A header that is known to reside within a given module,
/// whether it was included or excluded.
class KnownHeader {
@@ -176,7 +179,7 @@ class ModuleMap {
/// Whether this header is available in the module.
bool isAvailable() const {
- return getModule()->isAvailable();
+ return getRole() != ExcludedHeader && getModule()->isAvailable();
}
/// Whether this header is accessible from the specified module.
@@ -691,9 +694,6 @@ class ModuleMap {
void addHeader(Module *Mod, Module::Header Header,
ModuleHeaderRole Role, bool Imported = false);
- /// Marks this header as being excluded from the given module.
- void excludeHeader(Module *Mod, Module::Header Header);
-
/// Parse the given module map file, and record any modules we
/// encounter.
///
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 73f9678834969..bd69d8f697de8 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1344,7 +1344,7 @@ bool HeaderSearch::isFileMultipleIncludeGuarded(const FileEntry *File) {
void HeaderSearch::MarkFileModuleHeader(const FileEntry *FE,
ModuleMap::ModuleHeaderRole Role,
bool isCompilingModuleHeader) {
- bool isModularHeader = !(Role & ModuleMap::TextualHeader);
+ bool isModularHeader = ModuleMap::isModular(Role);
// Don't mark the file info as non-external if there's nothing to change.
if (!isCompilingModuleHeader) {
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index cbd3303f36636..2d8970516a5d2 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -75,7 +75,6 @@ void ModuleMap::addLinkAsDependency(Module *Mod) {
Module::HeaderKind ModuleMap::headerRoleToKind(ModuleHeaderRole Role) {
switch ((int)Role) {
- default: llvm_unreachable("unknown header role");
case NormalHeader:
return Module::HK_Normal;
case PrivateHeader:
@@ -84,7 +83,10 @@ Module::HeaderKind ModuleMap::headerRoleToKind(ModuleHeaderRole Role) {
return Module::HK_Textual;
case PrivateHeader | TextualHeader:
return Module::HK_PrivateTextual;
+ case ExcludedHeader:
+ return Module::HK_Excluded;
}
+ llvm_unreachable("unknown header role");
}
ModuleMap::ModuleHeaderRole
@@ -99,11 +101,15 @@ ModuleMap::headerKindToRole(Module::HeaderKind Kind) {
case Module::HK_PrivateTextual:
return ModuleHeaderRole(PrivateHeader | TextualHeader);
case Module::HK_Excluded:
- llvm_unreachable("unexpected header kind");
+ return ExcludedHeader;
}
llvm_unreachable("unknown header kind");
}
+bool ModuleMap::isModular(ModuleHeaderRole Role) {
+ return !(Role & (ModuleMap::TextualHeader | ModuleMap::ExcludedHeader));
+}
+
Module::ExportDecl
ModuleMap::resolveExport(Module *Mod,
const Module::UnresolvedExportDecl &Unresolved,
@@ -264,10 +270,7 @@ void ModuleMap::resolveHeader(Module *Mod,
} else {
Module::Header H = {Header.FileName, std::string(RelativePathName.str()),
*File};
- if (Header.Kind == Module::HK_Excluded)
- excludeHeader(Mod, H);
- else
- addHeader(Mod, H, headerKindToRole(Header.Kind));
+ addHeader(Mod, H, headerKindToRole(Header.Kind));
}
} else if (Header.HasBuiltinHeader && !Header.Size && !Header.ModTime) {
// There's a builtin header but no corresponding on-disk header. Assume
@@ -489,6 +492,12 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
HeadersMap::iterator Known = findKnownHeader(File);
if (Known != Headers.end()) {
for (const KnownHeader &Header : Known->second) {
+ // Excluded headers don't really belong to a module.
+ if (Header.getRole() == ModuleMap::ExcludedHeader) {
+ Excluded = true;
+ continue;
+ }
+
// Remember private headers for later printing of a diagnostic.
if (violatesPrivateInclude(RequestingModule, File, Header)) {
Private = Header.getModule();
@@ -562,6 +571,11 @@ static bool isBetterKnownHeader(const ModuleMap::KnownHeader &New,
(Old.getRole() & ModuleMap::TextualHeader))
return !(New.getRole() & ModuleMap::TextualHeader);
+ // Prefer a non-excluded header over an excluded header.
+ if ((New.getRole() == ModuleMap::ExcludedHeader) !=
+ (Old.getRole() == ModuleMap::ExcludedHeader))
+ return New.getRole() != ModuleMap::ExcludedHeader;
+
// Don't have a reason to choose between these. Just keep the first one.
return false;
}
@@ -570,8 +584,7 @@ ModuleMap::KnownHeader ModuleMap::findModuleForHeader(const FileEntry *File,
bool AllowTextual,
bool AllowExcluded) {
auto MakeResult = [&](ModuleMap::KnownHeader R) -> ModuleMap::KnownHeader {
- if ((!AllowTextual && R.getRole() & ModuleMap::TextualHeader) ||
- (!AllowExcluded && R.getRole() & ModuleMap::ExcludedHeader))
+ if (!AllowTextual && R.getRole() & ModuleMap::TextualHeader)
return {};
return R;
};
@@ -581,6 +594,9 @@ ModuleMap::KnownHeader ModuleMap::findModuleForHeader(const FileEntry *File,
ModuleMap::KnownHeader Result;
// Iterate over all modules that 'File' is part of to find the best fit.
for (KnownHeader &H : Known->second) {
+ // Cannot use a module if the header is excluded in it.
+ if (!AllowExcluded && H.getRole() == ModuleMap::ExcludedHeader)
+ continue;
// Prefer a header from the source module over all others.
if (H.getModule()->getTopLevelModule() == SourceModule)
return MakeResult(H);
@@ -1267,18 +1283,6 @@ void ModuleMap::addHeader(Module *Mod, Module::Header Header,
Cb->moduleMapAddHeader(Header.Entry->getName());
}
-void ModuleMap::excludeHeader(Module *Mod, Module::Header Header) {
- KnownHeader KH(Mod, ModuleHeaderRole::ExcludedHeader);
-
- // Add this as a known header so we won't implicitly add it to any
- // umbrella directory module.
- // FIXME: Should we only exclude it from umbrella modules within the
- // specified module?
- Headers[Header.Entry].push_back(KH);
-
- Mod->Headers[Module::HK_Excluded].push_back(std::move(Header));
-}
-
Optional<FileEntryRef>
ModuleMap::getContainingModuleMapFile(const Module *Module) const {
if (Module->DefinitionLoc.isInvalid())
@@ -2346,6 +2350,7 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
SourceLocation LeadingLoc) {
// We've already consumed the first token.
ModuleMap::ModuleHeaderRole Role = ModuleMap::NormalHeader;
+
if (LeadingToken == MMToken::PrivateKeyword) {
Role = ModuleMap::PrivateHeader;
// 'private' may optionally be followed by 'textual'.
@@ -2353,6 +2358,8 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
LeadingToken = Tok.Kind;
consumeToken();
}
+ } else if (LeadingToken == MMToken::ExcludeKeyword) {
+ Role = ModuleMap::ExcludedHeader;
}
if (LeadingToken == MMToken::TextualKeyword)
@@ -2386,9 +2393,7 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
Header.FileName = std::string(Tok.getString());
Header.FileNameLoc = consumeToken();
Header.IsUmbrella = LeadingToken == MMToken::UmbrellaKeyword;
- Header.Kind =
- (LeadingToken == MMToken::ExcludeKeyword ? Module::HK_Excluded
- : Map.headerRoleToKind(Role));
+ Header.Kind = Map.headerRoleToKind(Role);
// Check whether we already have an umbrella.
if (Header.IsUmbrella && ActiveModule->Umbrella) {
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 48328f60a0e7b..4a43cfdb0448c 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -910,6 +910,10 @@ Preprocessor::getHeaderToIncludeForDiagnostics(SourceLocation IncLoc,
continue;
}
+ // Don't suggest explicitly excluded headers.
+ if (Header.getRole() == ModuleMap::ExcludedHeader)
+ continue;
+
// We'll suggest including textual headers below if they're
// include-guarded.
if (Header.getRole() & ModuleMap::TextualHeader)
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 35d7055f8530b..45a6a474f1d93 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -1919,7 +1919,7 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
Module::Header H = {std::string(key.Filename), "",
*FileMgr.getFile(Filename)};
ModMap.addHeader(Mod, H, HeaderRole, /*Imported*/true);
- HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+ HFI.isModuleHeader |= ModuleMap::isModular(HeaderRole);
}
// This HeaderFileInfo was externally loaded.
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 7bd6142fcf2d0..73441a6efbc97 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1829,8 +1829,6 @@ namespace {
}
};
- // FIXME: If the header is excluded, we should write out some
- // record of that fact.
for (auto ModInfo : Data.KnownHeaders)
EmitModule(ModInfo.getModule(), ModInfo.getRole());
if (Data.Unresolved.getPointer())
diff --git a/clang/test/Modules/exclude-header-fw-umbrella.m b/clang/test/Modules/exclude-header-fw-umbrella.m
new file mode 100644
index 0000000000000..6b718abb31df6
--- /dev/null
+++ b/clang/test/Modules/exclude-header-fw-umbrella.m
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- frameworks/A.framework/Modules/module.modulemap
+framework module A {
+ umbrella header "A.h"
+ exclude header "Excluded.h"
+
+ module Excluded {
+ header "Excluded.h"
+ }
+}
+//--- frameworks/A.framework/Headers/A.h
+#import <A/Sub.h>
+//--- frameworks/A.framework/Headers/Sub.h
+//--- frameworks/A.framework/Headers/Excluded.h
+#import <A/Sub.h>
+ at interface I
+ at end
+
+//--- tu.m
+#import <A/Excluded.h>
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -iframework %t/frameworks -fsyntax-only %t/tu.m -verify
+// expected-no-diagnostics
More information about the cfe-commits
mailing list