jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, rsmith.
Herald added a subscriber: ributzka.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This is a follow-up to D134224 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D135381
Files:
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
clang/test/Modules/exclude-header-fw-umbrella.m
Index: clang/test/Modules/exclude-header-fw-umbrella.m
===================================================================
--- /dev/null
+++ 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>
+@interface I
+@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
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1829,8 +1829,6 @@
}
};
- // 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())
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1919,7 +1919,7 @@
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.
Index: clang/lib/Lex/PPDirectives.cpp
===================================================================
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -910,6 +910,10 @@
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)
Index: clang/lib/Lex/ModuleMap.cpp
===================================================================
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -75,7 +75,6 @@
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 @@
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 @@
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 @@
} 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 @@
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 @@
(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 @@
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 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);
@@ -1265,18 +1281,6 @@
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));
-}
-
const FileEntry *
ModuleMap::getContainingModuleMapFile(const Module *Module) const {
if (Module->DefinitionLoc.isInvalid())
@@ -2306,6 +2310,7 @@
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'.
@@ -2313,6 +2318,8 @@
LeadingToken = Tok.Kind;
consumeToken();
}
+ } else if (LeadingToken == MMToken::ExcludeKeyword) {
+ Role = ModuleMap::ExcludedHeader;
}
if (LeadingToken == MMToken::TextualKeyword)
@@ -2346,9 +2353,7 @@
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) {
Index: clang/lib/Lex/HeaderSearch.cpp
===================================================================
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1351,7 +1351,7 @@
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) {
Index: clang/include/clang/Lex/ModuleMap.h
===================================================================
--- clang/include/clang/Lex/ModuleMap.h
+++ clang/include/clang/Lex/ModuleMap.h
@@ -152,6 +152,9 @@
/// 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 @@
/// 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.
@@ -682,9 +685,6 @@
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.
///
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
