[modules] Support combining 'textual' with 'private'.

llvm-svn: 220589

clang/docs/Modules.rst

@@ -445,9 +445,8 @@ A header declaration specifies that a particular header is associated with the e
 .. parsed-literal::
 
   *header-declaration*:
-    ``umbrella``:sub:`opt` ``header`` *string-literal*
+    ``private``:sub:`opt` ``textual``:sub:`opt` ``header`` *string-literal*
-    ``private`` ``header`` *string-literal*
+    ``umbrella`` ``header`` *string-literal*
-    ``textual`` ``header`` *string-literal*
     ``exclude`` ``header`` *string-literal*
 
 A header declaration that does not contain ``exclude`` nor ``textual`` specifies a header that contributes to the enclosing module. Specifically, when the module is built, the named header will be parsed and its declarations will be (logically) placed into the enclosing submodule.
@@ -464,14 +463,14 @@ A header with the ``private`` specifier may not be included from outside the mod
 
 A header with the ``textual`` specifier will not be included when the module is built, and will be textually included if it is named by a ``#include`` directive. However, it is considered to be part of the module for the purpose of checking *use-declaration*\s.
 
-A header with the ``exclude`` specifier is excluded from the module. It will not be included when the module is built, nor will it be considered to be part of the module.
+A header with the ``exclude`` specifier is excluded from the module. It will not be included when the module is built, nor will it be considered to be part of the module, even if an ``umbrella`` header or directory would otherwise make it part of the module.
 
-**Example**: The C header ``assert.h`` is an excellent candidate for an excluded header, because it is meant to be included multiple times (possibly with different ``NDEBUG`` settings).
+**Example**: The C header ``assert.h`` is an excellent candidate for a textual header, because it is meant to be included multiple times (possibly with different ``NDEBUG`` settings). However, declarations within it should typically be split into a separate modular header.
 
 .. parsed-literal::
 
   module std [system] {
-    exclude header "assert.h"
+    textual header "assert.h"
   }
 
 A given header shall not be referenced by more than one *header-declaration*.

clang/include/clang/Basic/Module.h

@@ -84,9 +84,6 @@ public:
   /// \brief The headers that are part of this module.
   SmallVector<const FileEntry *, 2> NormalHeaders;
 
-  /// \brief The headers that are explicitly excluded from this module.
-  SmallVector<const FileEntry *, 2> ExcludedHeaders;
-
   /// \brief The headers that are logically part of this module but
   /// must be textually included.
   SmallVector<const FileEntry *, 2> TextualHeaders;
@@ -94,6 +91,13 @@ public:
   /// \brief The headers that are private to this module.
   SmallVector<const FileEntry *, 2> PrivateHeaders;
 
+  /// \brief The headers that are private to this module and are to be
+  /// included textually.
+  SmallVector<const FileEntry *, 2> PrivateTextualHeaders;
+
+  /// \brief The headers that are explicitly excluded from this module.
+  SmallVector<const FileEntry *, 2> ExcludedHeaders;
+
   /// \brief Information about a header directive as found in the module map
   /// file.
   struct HeaderDirective {

clang/include/clang/Lex/ModuleMap.h

@@ -65,20 +65,21 @@ private:
   llvm::StringMap<Module *> Modules;
 
 public:
-  /// \brief Describes the role of a module header.
+  /// \brief Flags describing the role of a module header.
   enum ModuleHeaderRole {
     /// \brief This header is normally included in the module.
-    NormalHeader,
+    NormalHeader  = 0x0,
     /// \brief This header is included but private.
-    PrivateHeader,
+    PrivateHeader = 0x1,
     /// \brief This header is part of the module (for layering purposes) but
     /// should be textually included.
-    TextualHeader,
+    TextualHeader = 0x2,
     // Caution: Adding an enumerator needs other changes.
     // Adjust the number of bits for KnownHeader::Storage.
     // Adjust the bitfield HeaderFileInfo::HeaderRole size.
     // Adjust the HeaderFileInfoTrait::ReadData streaming.
     // Adjust the HeaderFileInfoTrait::EmitData streaming.
+    // Adjust ModuleMap::addHeader.
   };
 
   /// \brief A header that is known to reside within a given module,

clang/include/clang/Serialization/ASTBitCodes.h

@@ -645,6 +645,9 @@ namespace clang {
       /// \brief Specifies a header that is part of the module but must be
       /// textually included.
       SUBMODULE_TEXTUAL_HEADER = 14,
+      /// \brief Specifies a header that is private to this submodule but
+      /// must be textually included.
+      SUBMODULE_PRIVATE_TEXTUAL_HEADER = 15,
     };
 
     /// \brief Record types used within a comments block.

clang/lib/Lex/ModuleMap.cpp

@@ -202,7 +202,7 @@ ModuleMap::findHeaderInUmbrellaDirs(const FileEntry *File,
   return KnownHeader();
 }
 
-// Returns 'true' if 'RequestingModule directly uses 'RequestedModule'.
+// Returns true if RequestingModule directly uses RequestedModule.
 static bool directlyUses(const Module *RequestingModule,
                          const Module *RequestedModule) {
   return std::find(RequestingModule->DirectUses.begin(),
@@ -214,19 +214,19 @@ static bool violatesPrivateInclude(Module *RequestingModule,
                                    const FileEntry *IncFileEnt,
                                    ModuleMap::ModuleHeaderRole Role,
                                    Module *RequestedModule) {
-  #ifndef NDEBUG
+  bool IsPrivateRole = Role & ModuleMap::PrivateHeader;
+#ifndef NDEBUG
   // Check for consistency between the module header role
   // as obtained from the lookup and as obtained from the module.
   // This check is not cheap, so enable it only for debugging.
-  SmallVectorImpl<const FileEntry *> &PvtHdrs
+  bool IsPrivate = false;
-      = RequestedModule->PrivateHeaders;
+  for (auto *Hdrs : {&RequestedModule->PrivateHeaders,
-  SmallVectorImpl<const FileEntry *>::iterator Look
+                     &RequestedModule->PrivateTextualHeaders})
-      = std::find(PvtHdrs.begin(), PvtHdrs.end(), IncFileEnt);
+    IsPrivate |=
-  bool IsPrivate = Look != PvtHdrs.end();
+        std::find(Hdrs->begin(), Hdrs->end(), IncFileEnt) != Hdrs->end();
-  assert((IsPrivate && Role == ModuleMap::PrivateHeader)
+  assert(IsPrivate == IsPrivateRole && "inconsistent headers and roles");
-               || (!IsPrivate && Role != ModuleMap::PrivateHeader));
+#endif
-  #endif
+  return IsPrivateRole &&
-  return Role == ModuleMap::PrivateHeader &&
          RequestedModule->getTopLevelModule() != RequestingModule;
 }
 
@@ -316,13 +316,13 @@ ModuleMap::findModuleForHeader(const FileEntry *File,
   HeadersMap::iterator Known = findKnownHeader(File);
 
   auto MakeResult = [&](ModuleMap::KnownHeader R) -> ModuleMap::KnownHeader {
-    if (!IncludeTextualHeaders && R.getRole() == ModuleMap::TextualHeader)
+    if (!IncludeTextualHeaders && (R.getRole() & ModuleMap::TextualHeader))
       return ModuleMap::KnownHeader();
     return R;
   };
 
   if (Known != Headers.end()) {
-    ModuleMap::KnownHeader Result = KnownHeader();
+    ModuleMap::KnownHeader Result;
 
     // Iterate over all modules that 'File' is part of to find the best fit.
     for (SmallVectorImpl<KnownHeader>::iterator I = Known->second.begin(),
@@ -343,14 +343,9 @@ ModuleMap::findModuleForHeader(const FileEntry *File,
           !directlyUses(RequestingModule, I->getModule()))
         continue;
 
-      Result = *I;
+      // Prefer a public header over a private header.
-      // If 'File' is a public header of this module, this is as good as we
+      if (!Result || (Result.getRole() & ModuleMap::PrivateHeader))
-      // are going to get.
+        Result = *I;
-      // FIXME: If we have a RequestingModule, we should prefer the header from
-      // that module.
-      if (I->getRole() == ModuleMap::NormalHeader ||
-          I->getRole() == ModuleMap::TextualHeader)
-        break;
     }
     return MakeResult(Result);
   }
@@ -783,13 +778,12 @@ void ModuleMap::setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir) {
 
 void ModuleMap::addHeader(Module *Mod, const FileEntry *Header,
                           ModuleHeaderRole Role) {
-  if (Role == TextualHeader) {
+  auto HeaderLists = {&Mod->NormalHeaders, &Mod->PrivateHeaders,
-    Mod->TextualHeaders.push_back(Header);
+                      &Mod->TextualHeaders, &Mod->PrivateTextualHeaders};
-  } else {
+  assert(Role >= 0 && Role < HeaderLists.size() && "unknown header role");
-    if (Role == PrivateHeader)
+  HeaderLists.begin()[Role]->push_back(Header);
-      Mod->PrivateHeaders.push_back(Header);
+
-    else
+  if (!(Role & TextualHeader)) {
-      Mod->NormalHeaders.push_back(Header);
     bool isCompilingModuleHeader = Mod->getTopLevelModule() == CompilingModule;
     HeaderInfo.MarkFileModuleHeader(Header, Role, isCompilingModuleHeader);
   }
@@ -1475,16 +1469,9 @@ void ModuleMapParser::parseModuleDecl() {
       parseRequiresDecl();
       break;
 
-    case MMToken::TextualKeyword: {
+    case MMToken::TextualKeyword:
-      SourceLocation TextualLoc = consumeToken();
+      parseHeaderDecl(MMToken::TextualKeyword, consumeToken());
-      if (Tok.is(MMToken::HeaderKeyword)) {
-        parseHeaderDecl(MMToken::TextualKeyword, TextualLoc);
-      } else {
-        Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header)
-          << "textual";
-      }
       break;
-    }
 
     case MMToken::UmbrellaKeyword: {
       SourceLocation UmbrellaLoc = consumeToken();
@@ -1494,31 +1481,17 @@ void ModuleMapParser::parseModuleDecl() {
         parseUmbrellaDirDecl(UmbrellaLoc);
       break;
     }
-        
+
-    case MMToken::ExcludeKeyword: {
+    case MMToken::ExcludeKeyword:
-      SourceLocation ExcludeLoc = consumeToken();
+      parseHeaderDecl(MMToken::ExcludeKeyword, consumeToken());
-      if (Tok.is(MMToken::HeaderKeyword)) {
-        parseHeaderDecl(MMToken::ExcludeKeyword, ExcludeLoc);
-      } else {
-        Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header)
-          << "exclude";
-      }
       break;
-    }
+
-      
+    case MMToken::PrivateKeyword:
-    case MMToken::PrivateKeyword: {
+      parseHeaderDecl(MMToken::PrivateKeyword, consumeToken());
-      SourceLocation PrivateLoc = consumeToken();
-      if (Tok.is(MMToken::HeaderKeyword)) {
-        parseHeaderDecl(MMToken::PrivateKeyword, PrivateLoc);
-      } else {
-        Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header)
-          << "private";
-      }
       break;
-    }
+
-      
     case MMToken::HeaderKeyword:
-      parseHeaderDecl(MMToken::HeaderKeyword, SourceLocation());
+      parseHeaderDecl(MMToken::HeaderKeyword, consumeToken());
       break;
 
     case MMToken::LinkKeyword:
@@ -1673,16 +1646,37 @@ static void appendSubframeworkPaths(Module *Mod,
 /// \brief Parse a header declaration.
 ///
 ///   header-declaration:
-///     'exclude'[opt] 'header' string-literal
-///     'private'[opt] 'header' string-literal
 ///     'textual'[opt] 'header' string-literal
-///     'umbrella'[opt] 'header' string-literal
+///     'private' 'textual'[opt] 'header' string-literal
+///     'exclude' 'header' string-literal
+///     'umbrella' 'header' string-literal
 ///
 /// FIXME: Support 'private textual header'.
 void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
                                       SourceLocation LeadingLoc) {
-  assert(Tok.is(MMToken::HeaderKeyword));
+  // We've already consumed the first token.
-  consumeToken();
+  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();
+    }
+  }
+  if (LeadingToken == MMToken::TextualKeyword)
+    Role = ModuleMap::ModuleHeaderRole(Role | ModuleMap::TextualHeader);
+
+  if (LeadingToken != MMToken::HeaderKeyword) {
+    if (!Tok.is(MMToken::HeaderKeyword)) {
+      Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header)
+          << (LeadingToken == MMToken::PrivateKeyword ? "private" :
+              LeadingToken == MMToken::ExcludeKeyword ? "exclude" :
+              LeadingToken == MMToken::TextualKeyword ? "textual" : "umbrella");
+      return;
+    }
+    consumeToken();
+  }
 
   // Parse the header name.
   if (!Tok.is(MMToken::StringLiteral)) {
@@ -1770,21 +1764,13 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
     } else if (LeadingToken == MMToken::ExcludeKeyword) {
       Map.excludeHeader(ActiveModule, File);
     } else {
-      // Record this header.
-      ModuleMap::ModuleHeaderRole Role = ModuleMap::NormalHeader;
-      if (LeadingToken == MMToken::PrivateKeyword)
-        Role = ModuleMap::PrivateHeader;
-      else if (LeadingToken == MMToken::TextualKeyword)
-        Role = ModuleMap::TextualHeader;
-      else
-        assert(LeadingToken == MMToken::HeaderKeyword);
-
       // If there is a builtin counterpart to this file, add it now, before
       // the "real" header, so we build the built-in one first when building
       // the module.
       if (BuiltinFile)
         Map.addHeader(ActiveModule, BuiltinFile, Role);
 
+      // Record this header.
       Map.addHeader(ActiveModule, File, Role);
     }
   } else if (LeadingToken != MMToken::ExcludeKeyword) {

clang/lib/Serialization/ASTReader.cpp

@@ -4461,26 +4461,19 @@ ASTReader::ReadSubmoduleBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
       break;
     }
         
-    case SUBMODULE_HEADER: {
+    case SUBMODULE_HEADER:
-      // We lazily associate headers with their modules via the HeaderInfoTable.
+    case SUBMODULE_EXCLUDED_HEADER:
+    case SUBMODULE_PRIVATE_HEADER:
+      // We lazily associate headers with their modules via the HeaderInfo table.
       // FIXME: Re-evaluate this section; maybe only store InputFile IDs instead
       // of complete filenames or remove it entirely.
-      break;      
+      break;
-    }
 
-    case SUBMODULE_EXCLUDED_HEADER: {
+    case SUBMODULE_TEXTUAL_HEADER:
-      // We lazily associate headers with their modules via the HeaderInfoTable.
+    case SUBMODULE_PRIVATE_TEXTUAL_HEADER:
-      // FIXME: Re-evaluate this section; maybe only store InputFile IDs instead
+      // FIXME: Textual headers are not marked in the HeaderInfo table. Load
-      // of complete filenames or remove it entirely.
+      // them here.
-      break;      
+      break;
-    }
-
-    case SUBMODULE_PRIVATE_HEADER: {
-      // We lazily associate headers with their modules via the HeaderInfoTable.
-      // FIXME: Re-evaluate this section; maybe only store InputFile IDs instead
-      // of complete filenames or remove it entirely.
-      break;      
-    }
 
     case SUBMODULE_TOPHEADER: {
       CurrentModule->addTopHeaderFilename(Blob);

clang/lib/Serialization/ASTWriter.cpp

@@ -2370,7 +2370,7 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
   }
   
   // Enter the submodule description block.
-  Stream.EnterSubblock(SUBMODULE_BLOCK_ID, /*bits for abbreviations*/4);
+  Stream.EnterSubblock(SUBMODULE_BLOCK_ID, /*bits for abbreviations*/5);
   
   // Write the abbreviations needed for the submodules block.
   using namespace llvm;
@@ -2430,6 +2430,11 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
   unsigned PrivateHeaderAbbrev = Stream.EmitAbbrev(Abbrev);
 
+  Abbrev = new BitCodeAbbrev();
+  Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_PRIVATE_TEXTUAL_HEADER));
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
+  unsigned PrivateTextualHeaderAbbrev = Stream.EmitAbbrev(Abbrev);
+
   Abbrev = new BitCodeAbbrev();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_LINK_LIBRARY));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
@@ -2504,40 +2509,25 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
     }
 
     // Emit the headers.
-    for (unsigned I = 0, N = Mod->NormalHeaders.size(); I != N; ++I) {
+    struct {
+      unsigned Kind;
+      unsigned Abbrev;
+      ArrayRef<const FileEntry*> Headers;
+    } HeaderLists[] = {
+      {SUBMODULE_HEADER, HeaderAbbrev, Mod->NormalHeaders},
+      {SUBMODULE_TEXTUAL_HEADER, TextualHeaderAbbrev, Mod->TextualHeaders},
+      {SUBMODULE_PRIVATE_HEADER, PrivateHeaderAbbrev, Mod->PrivateHeaders},
+      {SUBMODULE_PRIVATE_TEXTUAL_HEADER, PrivateTextualHeaderAbbrev,
+       Mod->PrivateTextualHeaders},
+      {SUBMODULE_EXCLUDED_HEADER, ExcludedHeaderAbbrev, Mod->ExcludedHeaders},
+      {SUBMODULE_TOPHEADER, TopHeaderAbbrev,
+       Mod->getTopHeaders(PP->getFileManager())}
+    };
+    for (auto &HL : HeaderLists) {
       Record.clear();
-      Record.push_back(SUBMODULE_HEADER);
+      Record.push_back(HL.Kind);
-      Stream.EmitRecordWithBlob(HeaderAbbrev, Record, 
+      for (auto *H : HL.Headers)
-                                Mod->NormalHeaders[I]->getName());
+        Stream.EmitRecordWithBlob(HL.Abbrev, Record, H->getName());
-    }
-    // Emit the excluded headers.
-    for (unsigned I = 0, N = Mod->ExcludedHeaders.size(); I != N; ++I) {
-      Record.clear();
-      Record.push_back(SUBMODULE_EXCLUDED_HEADER);
-      Stream.EmitRecordWithBlob(ExcludedHeaderAbbrev, Record, 
-                                Mod->ExcludedHeaders[I]->getName());
-    }
-    // Emit the textual headers.
-    for (unsigned I = 0, N = Mod->TextualHeaders.size(); I != N; ++I) {
-      Record.clear();
-      Record.push_back(SUBMODULE_TEXTUAL_HEADER);
-      Stream.EmitRecordWithBlob(TextualHeaderAbbrev, Record, 
-                                Mod->TextualHeaders[I]->getName());
-    }
-    // Emit the private headers.
-    for (unsigned I = 0, N = Mod->PrivateHeaders.size(); I != N; ++I) {
-      Record.clear();
-      Record.push_back(SUBMODULE_PRIVATE_HEADER);
-      Stream.EmitRecordWithBlob(PrivateHeaderAbbrev, Record, 
-                                Mod->PrivateHeaders[I]->getName());
-    }
-    ArrayRef<const FileEntry *>
-      TopHeaders = Mod->getTopHeaders(PP->getFileManager());
-    for (unsigned I = 0, N = TopHeaders.size(); I != N; ++I) {
-      Record.clear();
-      Record.push_back(SUBMODULE_TOPHEADER);
-      Stream.EmitRecordWithBlob(TopHeaderAbbrev, Record,
-                                TopHeaders[I]->getName());
     }
 
     // Emit the imports. 

clang/test/Modules/Inputs/declare-use/m.h

@@ -0,0 +1,8 @@
+#ifdef GIMME_AN_M
+
+#ifndef M_H
+#define M_H
+const int m = 42;
+#endif
+
+#endif

clang/test/Modules/Inputs/declare-use/m2.h

@@ -0,0 +1 @@
+#include "m.h"

clang/test/Modules/Inputs/declare-use/module.map

@@ -61,5 +61,10 @@ module XL {
   textual header "l.h"
 }
 
+module XM {
+  private textual header "m.h"
+  textual header "m2.h"
+}
+
 module XS {
 }

clang/test/Modules/textual-headers.cpp

@@ -1,6 +1,6 @@
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodule-maps -fmodules-cache-path=%t -fmodules-strict-decluse -fmodule-name=XG -I %S/Inputs/declare-use %s -verify
-// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fmodules-strict-decluse -fmodule-name=XG -I %S/Inputs/declare-use %s -verify
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fmodules-strict-decluse -fmodule-name=XG -I %S/Inputs/declare-use %s -verify -fno-modules-error-recovery
 
 #define GIMME_A_K
 #include "k.h"
@@ -8,4 +8,11 @@
 #define GIMME_AN_L
 #include "l.h" // expected-error {{module XG does not depend on a module exporting 'l.h'}}
 
+#include "m2.h" // expected-error {{module XG does not depend on a module exporting 'm2.h'}}
+const int use_m = m; // expected-error {{undeclared identifier}}
+
+#define GIMME_AN_M
+#include "m.h" // expected-error {{use of private header from outside its module: 'm.h'}}
+const int use_m_2 = m;
+
 const int g = k + l;