[clang][modules] Fix handling of `ModuleHeaderRole::ExcludedHeader`

[jansvoboda11]

Summary

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.

[jansvoboda11]

clang/lib/Lex/ModuleMap.cpp

598

This is what actually fixes the attached test case. MakeResult was too late to check for excluded headers (in the if (H.getModule()->getTopLevelModule() == SourceModule) condition) - we'd end up returning {} and skipping remaining module associations of the header file. This caused the ObjC interface to be associated with the wrong module in the PCM, causing "interface redefinition" errors.

[Bigcheese]

lgtm

clang/include/clang/Lex/ModuleMap.h

public:
   };
 
   /// Convert a header kind to a role. Requires Kind to not be HK_Excluded.
   static ModuleHeaderRole headerKindToRole(Module::HeaderKind Kind);
 
   /// 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 {
     llvm::PointerIntPair<Module *, 3, ModuleHeaderRole> Storage;
 
   public:
     KnownHeader() : Storage(nullptr, NormalHeader) {}
     KnownHeader(Module *M, ModuleHeaderRole Role) : Storage(M, Role) {}
 
     friend bool operator==(const KnownHeader &A, const KnownHeader &B) {
       return A.Storage == B.Storage;
     }
     friend bool operator!=(const KnownHeader &A, const KnownHeader &B) {
       return A.Storage != B.Storage;
     }
 
     /// Retrieve the module the header is stored in.
     Module *getModule() const { return Storage.getPointer(); }
 
     /// The role of this header within the module.
     ModuleHeaderRole getRole() const { return Storage.getInt(); }
 
     /// 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.
     bool isAccessibleFrom(Module *M) const {
       return !(getRole() & PrivateHeader) ||
              (M && M->getTopLevelModule() == getModule()->getTopLevelModule());
     }
 
void setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir,
                       const Twine &NameAsWritten,
                       const Twine &PathRelativeToRootModuleDirectory);
 
   /// Adds this header to the given module.
   /// \param Role The role of the header wrt the module.
   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.
   ///
   /// \param File The file to be parsed.
   ///
   /// \param IsSystem Whether this module map file is in a system header
   /// directory, and therefore should be considered a system module.
   ///

clang/lib/Lex/HeaderSearch.cpp

if (auto *HFI = getExistingFileInfo(File))
     return HFI->isPragmaOnce || HFI->ControllingMacro ||
            HFI->ControllingMacroID;
   return false;
 }
 
 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) {
     if (!isModularHeader)
       return;
     auto *HFI = getExistingFileInfo(FE);
     if (HFI && HFI->isModuleHeader)
       return;

clang/lib/Lex/ModuleMap.cpp

void ModuleMap::addLinkAsDependency(Module *Mod) {
   if (findModule(Mod->ExportAsModule))
     Mod->UseExportAsModuleLinkName = true;
   else
     PendingLinkAsModule[Mod->ExportAsModule].insert(Mod->Name);
 }
 
 Module::HeaderKind ModuleMap::headerRoleToKind(ModuleHeaderRole Role) {
   switch ((int)Role) {
-  default: llvm_unreachable("unknown header role");
   case NormalHeader:
     return Module::HK_Normal;
   case PrivateHeader:
     return Module::HK_Private;
   case TextualHeader:
     return Module::HK_Textual;
   case PrivateHeader | TextualHeader:
     return Module::HK_PrivateTextual;
+  case ExcludedHeader:
+    return Module::HK_Excluded;
   }
+  llvm_unreachable("unknown header role");
 }
 
 ModuleMap::ModuleHeaderRole
 ModuleMap::headerKindToRole(Module::HeaderKind Kind) {
   switch ((int)Kind) {
   case Module::HK_Normal:
     return NormalHeader;
   case Module::HK_Private:
     return PrivateHeader;
   case Module::HK_Textual:
     return TextualHeader;
   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,
                          bool Complain) const {
   // We may have just a wildcard.
   if (Unresolved.Id.empty()) {
     assert(Unresolved.Wildcard && "Invalid unresolved export");
     return Module::ExportDecl(nullptr, true);
if (Header.IsUmbrella) {
         Diags.Report(Header.FileNameLoc, diag::err_mmap_umbrella_clash)
           << UmbrellaMod->getFullModuleName();
       else
         // Record this umbrella header.
         setUmbrellaHeader(Mod, *File, Header.FileName, RelativePathName.str());
     } 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));
     }
   } else if (Header.HasBuiltinHeader && !Header.Size && !Header.ModTime) {
     // There's a builtin header but no corresponding on-disk header. Assume
     // this was supposed to modularize the builtin header alone.
   } else if (Header.Kind == Module::HK_Excluded) {
     // Ignore missing excluded header files. They're optional anyway.
   } else {
     // If we find a module that has a missing header, we mark this module as
void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule,
 
   bool Excluded = false;
   Module *Private = nullptr;
   Module *NotUsed = nullptr;
 
   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();
         continue;
       }
 
       // If uses need to be specified explicitly, we are only allowed to return
       // modules that are explicitly used by the requesting module.
if ((New.getRole() & ModuleMap::PrivateHeader) !=
       (Old.getRole() & ModuleMap::PrivateHeader))
     return !(New.getRole() & ModuleMap::PrivateHeader);
 
   // Prefer a non-textual header over a textual header.
   if ((New.getRole() & ModuleMap::TextualHeader) !=
       (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;
 }
 
 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;
   };
 
   HeadersMap::iterator Known = findKnownHeader(File);
   if (Known != Headers.end()) {
     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);
       if (!Result || isBetterKnownHeader(H, Result))
         Result = H;
     }
     return MakeResult(Result);
   }
HeaderInfo.MarkFileModuleHeader(Header.Entry, Role,
                                     isCompilingModuleHeader);
   }
 
   // Notify callbacks that we just added a new header.
   for (const auto &Cb : Callbacks)
     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())
     return None;
 
   return SourceMgr.getFileEntryRefForID(
       SourceMgr.getFileID(Module->DefinitionLoc));
 }

 ///     'exclude' 'header' string-literal
 ///     'umbrella' 'header' string-literal
 ///
 /// FIXME: Support 'private textual header'.
 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'.
     if (Tok.is(MMToken::TextualKeyword)) {
       LeadingToken = Tok.Kind;
       consumeToken();
     }
+  } else if (LeadingToken == MMToken::ExcludeKeyword) {
+    Role = ModuleMap::ExcludedHeader;
   }
 
   if (LeadingToken == MMToken::TextualKeyword)
     Role = ModuleMap::ModuleHeaderRole(Role | ModuleMap::TextualHeader);
 
   if (UsesRequiresExcludedHack.count(ActiveModule)) {
     // Mark this header 'textual' (see doc comment for
     // Module::UsesRequiresExcludedHack).
Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header)
       << "header";
     HadError = true;
     return;
   }
   Module::UnresolvedHeaderDirective Header;
   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) {
     Diags.Report(Header.FileNameLoc, diag::err_mmap_umbrella_clash)
       << ActiveModule->getFullModuleName();
     HadError = true;
     return;
   }

clang/lib/Lex/PPDirectives.cpp

for (auto Header : HeaderInfo.findAllModulesForHeader(FE)) {
         // It's in a private header; we can't #include it.
         // FIXME: If there's a public header in some module that re-exports it,
         // then we could suggest including that, but it's not clear that's the
         // expected way to make this entity visible.
         InPrivateHeader = true;
         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)
         continue;
 
       // If we have a module import syntax, we shouldn't include a header to
       // make a particular module visible. Let the caller know they should
       // suggest an import instead.

clang/lib/Serialization/ASTReader.cpp

while (d != End) {
 
     std::string Filename = std::string(key.Filename);
     if (key.Imported)
       Reader.ResolveImportedPath(M, Filename);
     // FIXME: NameAsWritten
     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.
   HFI.External = true;
   HFI.IsValid = true;
   return HFI;
 }
 

clang/lib/Serialization/ASTWriter.cpp

void EmitData(raw_ostream &Out, key_type_ref key,
       auto EmitModule = [&](Module *M, ModuleMap::ModuleHeaderRole Role) {
         if (uint32_t ModID = Writer.getLocalOrImportedSubmoduleID(M)) {
           uint32_t Value = (ModID << 3) | (unsigned)Role;
           assert((Value >> 3) == ModID && "overflow in header module info");
           LE.write<uint32_t>(Value);
         }
       };
 
-      // 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())
         EmitModule(Data.Unresolved.getPointer(), Data.Unresolved.getInt());
 
       assert(Out.tell() - Start == DataLen && "Wrong data length");
     }
 

clang/test/Modules/exclude-header-fw-umbrella.m

+// 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