Private Headers for Modules

[crowl]

Summary

This patch adds new private headers to the module map. Private
headers may be included from within the module, but not from outside
the module.

The patch does not provide any symbol export control, only inclusion
control. This capability enables a small step towards limiting
unintended use of a module/library.

Add a new declaration to module maps

private header "filename";

DOCUMENTATION

In docs/Modules.rst, document the declaration.

MODULE

Modify class Module to track private headers.

Add field PrivateHeaders to list the private headers.

Rename field Headers to NormalHeaders. Update uses to match.

Print private headers in Module::print.

MODULE MAP

Modify class ModuleMap to keep the role of the header as an enum
rather than a boolean.

Add enum ModuleHeaderRole describing the role of the header, normal,
private, and excluded.

To nested class KnownHeader, change bool Excluded to ModuleHeaderRole
Role.

Several functions that take or return a bool Excluded parameter, now
take a ModuleHeaderRole Role parameter. Several functions that did
not take a parameter, because they only dealt with normal headers,
now take an additional ModuleHeaderRole Role parameter.

Several that take that take or return Module* will instead take or
return KnownHeader, which provides access to the ModuleHeaderRole.

Add syntax for private headers. This syntax is the same as excluded
headers, but substituting the token "private" instead of "exclude".
Add MMToken::PrivateKeyword.

Change the parameters of parseHeaderDecl from two mutually exclusive
SourceLocations to a the leading token and the source location
of that token. Modify callers to match. Some checking need not
happen as a result. Infer the header role from the leading token.

HEADER SEARCH

Modify struct HeaderFileInfo to keep the role of the header.

Add field ModuleMap::ModuleHeaderRole HeaderRole : 2; constructed
with value ModuleMap::NormalHeader by default.

PREPROCESSOR

Modify In Preprocessor::LookupFile to check for inappropriate use
of private headers. This requires changing the parameter Module
**SuggestedModule to ModuleMap::KnownHeader *SuggestedModule.
To provide diagnostic line information, It also requires adding a
parameter SourceLocation FilenameLoc. Update callers to match.

FRONTEND ACTIONS

Make a note that private headers will not be top headers.

SERIALIZATION

Adjust serialization.

To enum SubmoduleRecordTypes add SUBMODULE_PRIVATE_HEADER = 13.

In ASTWriter::WriteSubmodules, add BitCodeAbbrev for
SUBMODULE_PRIVATE_HEADER. Emit PrivateHeaders names.

In ASTWriter.cpp'HeaderFileInfoTrait::EmitData, write HeaderRole
to flag byte. This addition uses both remaining bits in the byte.

In ASTReader.cpp'HeaderFileInfoTrait::ReadData: read HeaderRole
from flags; Pass HeaderRole to ModMap.addHeader.

In ASTReader.cpp'ASTReader::ReadSubmoduleBlock: Handle
SUBMODULE_PRIVATE_HEADER like the other headers.

TESTING

Add new module tests for private headers. These are pretty minimal.

[klimek]

lib/Lex/PPDirectives.cpp

573–582

This looks unintentionally left as a comment?

[doug.gregor]

This is looking really good. Only minor change requests above.

lib/Lex/PPDirectives.cpp

573–582

If this is really that expensive, it can go behind ENABLE_EXPENSIVE_CHECKS, but we'd rather not have it commented out.

test/Modules/private1.cpp

9

Please merge these three tests into a single ".cpp" file, since there's a nontrivial cost to testing each individual file.

Also, please add an #include "private.h" with an expected-error to test the new diagnostic.

[gribozavr]

include/clang/Lex/HeaderSearch.h

61

IIRC, this should be unsigned because MSVC does not like bitfields with signed enum types.

include/clang/Lex/ModuleMap.h

205–207

The name and \returns documentation now don't quite reflect the actual return value.

[crowl]

This diff addresses the previous comments and adds a new test case.

[doug.gregor]

This LGTM now, thanks!

[Eugene.Zelenko]

Committed in rL184471 and rL184472.

docs/Modules.rst

 
   ``config_macros`` ``export``     ``module``
   ``conflict``      ``framework``  ``requires``
-  ``exclude``       ``header``     ``umbrella``
-  ``explicit``      ``link``
+  ``exclude``       ``header``     ``private``
+  ``explicit``      ``link``       ``umbrella``
 
 Module map file
 ---------------

 
   *header-declaration*:
     ``umbrella``:sub:`opt` ``header`` *string-literal*
+    ``private`` ``header`` *string-literal*
     ``exclude`` ``header`` *string-literal*
 
 A header declaration that does not contain ``exclude`` 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.

     ``-Wincomplete-umbrella`` warning option to ask Clang to complain
     about headers not covered by the umbrella header or the module map.
 
+A header with the ``private`` specifier may not be included from outside the module itself.
+
 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.
 
 **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).

include/clang/Basic/DiagnosticLexKinds.td

   InGroup<IncompleteUmbrella>;
 def err_expected_id_building_module : Error<
   "expected a module name in '__building_module' expression">;
+def error_use_of_private_header_outside_module : Error<
+  "use of private header from outside its module: '%0'">;
   
 }

include/clang/Basic/Module.h

 
 public:
   /// \brief The headers that are part of this module.
-  SmallVector<const FileEntry *, 2> Headers;
+  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 private to this module.
+  llvm::SmallVector<const FileEntry *, 2> PrivateHeaders;
+
   /// \brief The set of language features required to use this module.
   ///
   /// If any of these features is not present, the \c IsAvailable bit

include/clang/Lex/DirectoryLookup.h

 
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Lex/ModuleMap.h"
 
 namespace clang {
 class HeaderMap;

   const FileEntry *LookupFile(StringRef Filename, HeaderSearch &HS,
                               SmallVectorImpl<char> *SearchPath,
                               SmallVectorImpl<char> *RelativePath,
-                              Module **SuggestedModule,
+                              ModuleMap::KnownHeader *SuggestedModule,
                               bool &InUserSpecifiedSystemFramework) const;
 
 private:

       StringRef Filename, HeaderSearch &HS,
       SmallVectorImpl<char> *SearchPath,
       SmallVectorImpl<char> *RelativePath,
-      Module **SuggestedModule,
+      ModuleMap::KnownHeader *SuggestedModule,
       bool &InUserSpecifiedSystemHeader) const;
 
 };

include/clang/Lex/HeaderSearch.h

 
   /// \brief Whether this header is part of the module that we are building.
   unsigned isCompilingModuleHeader : 1;
+
+  /// \brief Whether this header is part of the module that we are building.
+  /// This is an instance of ModuleMap::ModuleHeaderRole.
+  unsigned HeaderRole : 2;
   
   /// \brief Whether this structure is considered to already have been
   /// "resolved", meaning that it was loaded from the external source.

   HeaderFileInfo()
     : isImport(false), isPragmaOnce(false), DirInfo(SrcMgr::C_User), 
       External(false), isModuleHeader(false), isCompilingModuleHeader(false),
+      HeaderRole(ModuleMap::NormalHeader),
       Resolved(false), IndexHeaderMapHeader(false),
       NumIncludes(0), ControllingMacroID(0), ControllingMacro(0)  {}
 

     return isImport || isPragmaOnce || NumIncludes || ControllingMacro || 
       ControllingMacroID;
   }
+
+  /// \brief Get the HeaderRole properly typed.
+  ModuleMap::ModuleHeaderRole getHeaderRole() const {
+    return static_cast<ModuleMap::ModuleHeaderRole>(HeaderRole);
+  }
+
+  /// \brief Set the HeaderRole properly typed.
+  void setHeaderRole(ModuleMap::ModuleHeaderRole Role) {
+    HeaderRole = Role;
+  }
 };
 
 /// \brief An external source of header file information, which may supply

                               const FileEntry *CurFileEnt,
                               SmallVectorImpl<char> *SearchPath,
                               SmallVectorImpl<char> *RelativePath,
-                              Module **SuggestedModule,
+                              ModuleMap::KnownHeader *SuggestedModule,
                               bool SkipCache = false);
 
   /// \brief Look up a subframework for the specified \#include file.

       const FileEntry *RelativeFileEnt,
       SmallVectorImpl<char> *SearchPath,
       SmallVectorImpl<char> *RelativePath,
-      Module **SuggestedModule);
+      ModuleMap::KnownHeader *SuggestedModule);
 
   /// \brief Look up the specified framework name in our framework cache.
   /// \returns The DirectoryEntry it is in if we know, null otherwise.

   }
 
   /// \brief Mark the specified file as part of a module.
-  void MarkFileModuleHeader(const FileEntry *File, bool IsCompiledModuleHeader);
+  void MarkFileModuleHeader(const FileEntry *File,
+                            ModuleMap::ModuleHeaderRole Role,
+                            bool IsCompiledModuleHeader);
 
   /// \brief Increment the count for the number of times the specified
   /// FileEntry has been entered.

   /// \brief Retrieve the module that corresponds to the given file, if any.
   ///
   /// \param File The header that we wish to map to a module.
-  Module *findModuleForHeader(const FileEntry *File) const;
+  ModuleMap::KnownHeader findModuleForHeader(const FileEntry *File) const;
   
   /// \brief Read the contents of the given module map file.
   ///

include/clang/Lex/ModuleMap.h

   /// \brief The top-level modules that are known.
   llvm::StringMap<Module *> Modules;
 
+public:
+  /// \brief Describes the role of a module header.
+  enum ModuleHeaderRole {
+    /// \brief This header is normally included in the module.
+    NormalHeader,
+    /// \brief This header is included but private.
+    PrivateHeader,
+    /// \brief This header is explicitly excluded from the module.
+    ExcludedHeader
+    // 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.
+  };
+
   /// \brief A header that is known to reside within a given module,
   /// whether it was included or excluded.
   class KnownHeader {
-    llvm::PointerIntPair<Module *, 1, bool> Storage;
+    llvm::PointerIntPair<Module *, 2, ModuleHeaderRole> Storage;
 
   public:
-    KnownHeader() : Storage(0, false) { }
-    KnownHeader(Module *M, bool Excluded) : Storage(M, Excluded) { }
+    KnownHeader() : Storage(0, NormalHeader) { }
+    KnownHeader(Module *M, ModuleHeaderRole Role) : Storage(M, Role) { }
 
     /// \brief Retrieve the module the header is stored in.
     Module *getModule() const { return Storage.getPointer(); }
 
-    /// \brief Whether this header is explicitly excluded from the module.
-    bool isExcluded() const { return Storage.getInt(); }
+    /// \brief The role of this header within the module.
+    ModuleHeaderRole getRole() const { return Storage.getInt(); }
 
     /// \brief Whether this header is available in the module.
     bool isAvailable() const { 
-      return !isExcluded() && getModule()->isAvailable(); 
+      return getRole() != ExcludedHeader && getModule()->isAvailable(); 
     }
 
     // \brief Whether this known header is valid (i.e., it has an

     LLVM_EXPLICIT operator bool() const { return Storage.getPointer() != 0; }
   };
 
+private:
   typedef llvm::DenseMap<const FileEntry *, KnownHeader> HeadersMap;
 
-  /// \brief Mapping from each header to the module that owns the contents of the
+  /// \brief Mapping from each header to the module that owns the contents of
   /// that header.
   HeadersMap Headers;
   

   ///
   /// \param File The header file that is likely to be included.
   ///
-  /// \returns The module that owns the given header file, or null to indicate
+  /// \returns The module KnownHeader, which provides the module that owns the
+  /// given header file.  The KnownHeader is default constructed to indicate
   /// that no module owns this header file.
-  Module *findModuleForHeader(const FileEntry *File);
+  KnownHeader findModuleForHeader(const FileEntry *File);
 
   /// \brief Determine whether the given header is part of a module
   /// marked 'unavailable'.

   void setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir);
 
   /// \brief Adds this header to the given module.
-  /// \param Excluded Whether this header is explicitly excluded from the
-  /// module; otherwise, it's included in the module.
-  void addHeader(Module *Mod, const FileEntry *Header, bool Excluded);
+  /// \param Role The role of the header wrt the module.
+  void addHeader(Module *Mod, const FileEntry *Header,
+                 ModuleHeaderRole Role);
 
   /// \brief Parse the given module map file, and record any modules we 
   /// encounter.

include/clang/Lex/Preprocessor.h

 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/MacroInfo.h"
+#include "clang/Lex/ModuleMap.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/PTHLexer.h"
 #include "clang/Lex/PTHManager.h"

   ///
   /// Returns null on failure.  \p isAngled indicates whether the file
   /// reference is for system \#include's or not (i.e. using <> instead of "").
-  const FileEntry *LookupFile(StringRef Filename,
+  const FileEntry *LookupFile(SourceLocation FilenameLoc, StringRef Filename,
                               bool isAngled, const DirectoryLookup *FromDir,
                               const DirectoryLookup *&CurDir,
                               SmallVectorImpl<char> *SearchPath,
                               SmallVectorImpl<char> *RelativePath,
-                              Module **SuggestedModule,
+                              ModuleMap::KnownHeader *SuggestedModule,
                               bool SkipCache = false);
 
   /// GetCurLookup - The DirectoryLookup structure used to find the current

include/clang/Serialization/ASTBitCodes.h

       /// \brief Specifies a configuration macro for this module.
       SUBMODULE_CONFIG_MACRO = 11,
       /// \brief Specifies a conflict with another module.
-      SUBMODULE_CONFLICT = 12
+      SUBMODULE_CONFLICT = 12,
+      /// \brief Specifies a header that is private to this submodule.
+      SUBMODULE_PRIVATE_HEADER = 13
     };
 
     /// \brief Record types used within a comments block.

lib/Basic/Module.cpp

     OS << "\n";
   }
 
-  for (unsigned I = 0, N = Headers.size(); I != N; ++I) {
+  for (unsigned I = 0, N = NormalHeaders.size(); I != N; ++I) {
     OS.indent(Indent + 2);
     OS << "header \"";
-    OS.write_escaped(Headers[I]->getName());
+    OS.write_escaped(NormalHeaders[I]->getName());
     OS << "\"\n";
   }
 

     OS.write_escaped(ExcludedHeaders[I]->getName());
     OS << "\"\n";
   }
+
+  for (unsigned I = 0, N = PrivateHeaders.size(); I != N; ++I) {
+    OS.indent(Indent + 2);
+    OS << "private header \"";
+    OS.write_escaped(PrivateHeaders[I]->getName());
+    OS << "\"\n";
+  }
   
   for (submodule_const_iterator MI = submodule_begin(), MIEnd = submodule_end();
        MI != MIEnd; ++MI)

lib/Frontend/FrontendActions.cpp

     return;
 
   // Add includes for each of these headers.
-  for (unsigned I = 0, N = Module->Headers.size(); I != N; ++I) {
-    const FileEntry *Header = Module->Headers[I];
+  for (unsigned I = 0, N = Module->NormalHeaders.size(); I != N; ++I) {
+    const FileEntry *Header = Module->NormalHeaders[I];
     Module->addTopHeader(Header);
     addHeaderInclude(Header, Includes, LangOpts);
   }
+  // Note that Module->PrivateHeaders will not be a TopHeader.
 
   if (const FileEntry *UmbrellaHeader = Module->getUmbrellaHeader()) {
     Module->addTopHeader(UmbrellaHeader);

lib/Frontend/VerifyDiagnosticConsumer.cpp

 
         // Lookup file via Preprocessor, like a #include.
         const DirectoryLookup *CurDir;
-        const FileEntry *FE = PP->LookupFile(Filename, false, NULL, CurDir,
+        const FileEntry *FE = PP->LookupFile(Pos, Filename, false, NULL, CurDir,
                                              NULL, NULL, 0);
         if (!FE) {
           Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),

lib/Lex/HeaderSearch.cpp

     HeaderSearch &HS,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
-    Module **SuggestedModule,
+    ModuleMap::KnownHeader *SuggestedModule,
     bool &InUserSpecifiedSystemFramework) const {
   InUserSpecifiedSystemFramework = false;
 

     HeaderSearch &HS,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
-    Module **SuggestedModule,
+    ModuleMap::KnownHeader *SuggestedModule,
     bool &InUserSpecifiedSystemFramework) const
 {
   FileManager &FileMgr = HS.getFileMgr();

     const FileEntry *CurFileEnt,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
-    Module **SuggestedModule,
+    ModuleMap::KnownHeader *SuggestedModule,
     bool SkipCache)
 {
   if (SuggestedModule)
-    *SuggestedModule = 0;
+    *SuggestedModule = ModuleMap::KnownHeader();
     
   // If 'Filename' is absolute, check to see if it exists and no searching.
   if (llvm::sys::path::is_absolute(Filename)) {

                          const FileEntry *ContextFileEnt,
                          SmallVectorImpl<char> *SearchPath,
                          SmallVectorImpl<char> *RelativePath,
-                         Module **SuggestedModule) {
+                         ModuleMap::KnownHeader *SuggestedModule) {
   assert(ContextFileEnt && "No context file?");
 
   // Framework names must have a '/' in the filename.  Find it.

 }
 
 void HeaderSearch::MarkFileModuleHeader(const FileEntry *FE,
+                                        ModuleMap::ModuleHeaderRole Role,
                                         bool isCompilingModuleHeader) {
   if (FE->getUID() >= FileInfo.size())
     FileInfo.resize(FE->getUID()+1);

   HeaderFileInfo &HFI = FileInfo[FE->getUID()];
   HFI.isModuleHeader = true;
   HFI.isCompilingModuleHeader = isCompilingModuleHeader;
+  HFI.setHeaderRole(Role);
 }
 
 bool HeaderSearch::ShouldEnterIncludeFile(const FileEntry *File, bool isImport){

   } while (true);
 }
 
-Module *HeaderSearch::findModuleForHeader(const FileEntry *File) const {
+ModuleMap::KnownHeader
+HeaderSearch::findModuleForHeader(const FileEntry *File) const {
   if (ExternalSource) {
     // Make sure the external source has handled header info about this file,
     // which includes whether the file is part of a module.
     (void)getFileInfo(File);
   }
-  if (Module *Mod = ModMap.findModuleForHeader(File))
-    return Mod;
-  
-  return 0;
+  return ModMap.findModuleForHeader(File);
 }
 
 bool HeaderSearch::loadModuleMapFile(const FileEntry *File) {

lib/Lex/ModuleMap.cpp

            .Default(false);
 }
 
-Module *ModuleMap::findModuleForHeader(const FileEntry *File) {
+ModuleMap::KnownHeader ModuleMap::findModuleForHeader(const FileEntry *File) {
   HeadersMap::iterator Known = Headers.find(File);
   if (Known != Headers.end()) {
     // If a header is not available, don't report that it maps to anything.
     if (!Known->second.isAvailable())
-      return 0;
+      return KnownHeader();
 
-    return Known->second.getModule();
+    return Known->second;
   }
 
   // If we've found a builtin header within Clang's builtin include directory,

     if (Known != Headers.end()) {
       // If a header is not available, don't report that it maps to anything.
       if (!Known->second.isAvailable())
-        return 0;
+        return KnownHeader();
 
-      return Known->second.getModule();
+      return Known->second;
     }
   }
   

           UmbrellaDirs[SkippedDirs[I]] = Result;
       }
       
-      Headers[File] = KnownHeader(Result, /*Excluded=*/false);
+      Headers[File] = KnownHeader(Result, NormalHeader);
 
       // If a header corresponds to an unavailable module, don't report
       // that it maps to anything.
       if (!Result->isAvailable())
-        return 0;
+        return KnownHeader();
 
-      return Result;
+      return Headers[File];
     }
     
     SkippedDirs.push_back(Dir);

     Dir = SourceMgr->getFileManager().getDirectory(DirName);
   } while (Dir);
   
-  return 0;
+  return KnownHeader();
 }
 
 bool ModuleMap::isHeaderInUnavailableModule(const FileEntry *Header) const {

   
   // umbrella header "umbrella-header-name"
   Result->Umbrella = UmbrellaHeader;
-  Headers[UmbrellaHeader] = KnownHeader(Result, /*Excluded=*/false);
+  Headers[UmbrellaHeader] = KnownHeader(Result, NormalHeader);
   UmbrellaDirs[UmbrellaHeader->getDir()] = Result;
   
   // export *

 }
 
 void ModuleMap::setUmbrellaHeader(Module *Mod, const FileEntry *UmbrellaHeader){
-  Headers[UmbrellaHeader] = KnownHeader(Mod, /*Excluded=*/false);
+  Headers[UmbrellaHeader] = KnownHeader(Mod, NormalHeader);
   Mod->Umbrella = UmbrellaHeader;
   UmbrellaDirs[UmbrellaHeader->getDir()] = Mod;
 }

 }
 
 void ModuleMap::addHeader(Module *Mod, const FileEntry *Header,
-                          bool Excluded) {
-  if (Excluded) {
+                          ModuleHeaderRole Role) {
+  if (Role == ExcludedHeader) {
     Mod->ExcludedHeaders.push_back(Header);
   } else {
-    Mod->Headers.push_back(Header);
+    if (Role == PrivateHeader)
+      Mod->PrivateHeaders.push_back(Header);
+    else
+      Mod->NormalHeaders.push_back(Header);
     bool isCompilingModuleHeader = Mod->getTopLevelModule() == CompilingModule;
-    HeaderInfo.MarkFileModuleHeader(Header, isCompilingModuleHeader);
+    HeaderInfo.MarkFileModuleHeader(Header, Role, isCompilingModuleHeader);
   }
-  Headers[Header] = KnownHeader(Mod, Excluded);
+  Headers[Header] = KnownHeader(Mod, Role);
 }
 
 const FileEntry *

   while (const FileEntry *ExpansionFile
            = SrcMgr.getFileEntryForID(ExpansionFileID)) {
     // Find the module that owns this header (if any).
-    if (Module *Mod = findModuleForHeader(ExpansionFile))
+    if (Module *Mod = findModuleForHeader(ExpansionFile).getModule())
       return Mod;
     
     // No module owns this header, so look up the inclusion chain to see if

       LinkKeyword,
       ModuleKeyword,
       Period,
+      PrivateKeyword,
       UmbrellaKeyword,
       RequiresKeyword,
       Star,

     bool parseModuleId(ModuleId &Id);
     void parseModuleDecl();
     void parseRequiresDecl();
-    void parseHeaderDecl(SourceLocation UmbrellaLoc, SourceLocation ExcludeLoc);
+    void parseHeaderDecl(clang::MMToken::TokenKind,
+                         SourceLocation LeadingLoc);
     void parseUmbrellaDirDecl(SourceLocation UmbrellaLoc);
     void parseExportDecl();
     void parseLinkDecl();

                  .Case("header", MMToken::HeaderKeyword)
                  .Case("link", MMToken::LinkKeyword)
                  .Case("module", MMToken::ModuleKeyword)
+                 .Case("private", MMToken::PrivateKeyword)
                  .Case("requires", MMToken::RequiresKeyword)
                  .Case("umbrella", MMToken::UmbrellaKeyword)
                  .Default(MMToken::Identifier);

     case MMToken::UmbrellaKeyword: {
       SourceLocation UmbrellaLoc = consumeToken();
       if (Tok.is(MMToken::HeaderKeyword))
-        parseHeaderDecl(UmbrellaLoc, SourceLocation());
+        parseHeaderDecl(MMToken::UmbrellaKeyword, UmbrellaLoc);
       else
         parseUmbrellaDirDecl(UmbrellaLoc);
       break;

     case MMToken::ExcludeKeyword: {
       SourceLocation ExcludeLoc = consumeToken();
       if (Tok.is(MMToken::HeaderKeyword)) {
-        parseHeaderDecl(SourceLocation(), ExcludeLoc);
+        parseHeaderDecl(MMToken::ExcludeKeyword, ExcludeLoc);
       } else {
         Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header)
           << "exclude";

       break;
     }
       
+    case MMToken::PrivateKeyword: {
+      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(SourceLocation(), SourceLocation());
+      parseHeaderDecl(MMToken::HeaderKeyword, SourceLocation());
       break;
 
     case MMToken::LinkKeyword:

 ///   header-declaration:
 ///     'umbrella'[opt] 'header' string-literal
 ///     'exclude'[opt] 'header' string-literal
-void ModuleMapParser::parseHeaderDecl(SourceLocation UmbrellaLoc,
-                                      SourceLocation ExcludeLoc) {
+void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
+                                      SourceLocation LeadingLoc) {
   assert(Tok.is(MMToken::HeaderKeyword));
   consumeToken();
 
-  bool Umbrella = UmbrellaLoc.isValid();
-  bool Exclude = ExcludeLoc.isValid();
-  assert(!(Umbrella && Exclude) && "Cannot have both 'umbrella' and 'exclude'");
   // Parse the header name.
   if (!Tok.is(MMToken::StringLiteral)) {
     Diags.Report(Tok.getLocation(), diag::err_mmap_expected_header) 

   SourceLocation FileNameLoc = consumeToken();
   
   // Check whether we already have an umbrella.
-  if (Umbrella && ActiveModule->Umbrella) {
+  if (LeadingToken == MMToken::UmbrellaKeyword && ActiveModule->Umbrella) {
     Diags.Report(FileNameLoc, diag::err_mmap_umbrella_clash)
       << ActiveModule->getFullModuleName();
     HadError = true;

       // If this is a system module with a top-level header, this header
       // may have a counterpart (or replacement) in the set of headers
       // supplied by Clang. Find that builtin header.
-      if (ActiveModule->IsSystem && !Umbrella && BuiltinIncludeDir &&
-          BuiltinIncludeDir != Directory && isBuiltinHeader(FileName)) {
+      if (ActiveModule->IsSystem && LeadingToken != MMToken::UmbrellaKeyword &&
+          BuiltinIncludeDir && BuiltinIncludeDir != Directory &&
+          isBuiltinHeader(FileName)) {
         SmallString<128> BuiltinPathName(BuiltinIncludeDir->getName());
         llvm::sys::path::append(BuiltinPathName, FileName);
         BuiltinFile = SourceMgr.getFileManager().getFile(BuiltinPathName);

       Diags.Report(FileNameLoc, diag::err_mmap_header_conflict)
         << FileName << OwningModule.getModule()->getFullModuleName();
       HadError = true;
-    } else if (Umbrella) {
+    } else if (LeadingToken == MMToken::UmbrellaKeyword) {
       const DirectoryEntry *UmbrellaDir = File->getDir();
       if (Module *UmbrellaModule = Map.UmbrellaDirs[UmbrellaDir]) {
-        Diags.Report(UmbrellaLoc, diag::err_mmap_umbrella_clash)
+        Diags.Report(LeadingLoc, diag::err_mmap_umbrella_clash)
           << UmbrellaModule->getFullModuleName();
         HadError = true;
       } else {

       }
     } else {
       // Record this header.
-      Map.addHeader(ActiveModule, File, Exclude);
+      ModuleMap::ModuleHeaderRole Role = ModuleMap::NormalHeader;
+      if (LeadingToken == MMToken::ExcludeKeyword)
+        Role = ModuleMap::ExcludedHeader;
+      else if (LeadingToken == MMToken::PrivateKeyword)
+        Role = ModuleMap::PrivateHeader;
+      else
+        assert(LeadingToken == MMToken::HeaderKeyword);
+        
+      Map.addHeader(ActiveModule, File, Role);
       
       // If there is a builtin counterpart to this file, add it now.
       if (BuiltinFile)
-        Map.addHeader(ActiveModule, BuiltinFile, Exclude);
+        Map.addHeader(ActiveModule, BuiltinFile, Role);
     }
-  } else if (!Exclude) {
+  } else if (LeadingToken != MMToken::ExcludeKeyword) {
     // Ignore excluded header files. They're optional anyway.
     
     Diags.Report(FileNameLoc, diag::err_mmap_header_not_found)
-      << Umbrella << FileName;
+      << (LeadingToken == MMToken::UmbrellaKeyword) << FileName;
     HadError = true;
   }
 }

     case MMToken::ExplicitKeyword:
     case MMToken::ModuleKeyword:
     case MMToken::HeaderKeyword:
+    case MMToken::PrivateKeyword:
     case MMToken::UmbrellaKeyword:
     default:
       Diags.Report(Tok.getLocation(), diag::err_mmap_expected_inferred_member)

     case MMToken::LinkKeyword:
     case MMToken::LSquare:
     case MMToken::Period:
+    case MMToken::PrivateKeyword:
     case MMToken::RBrace:
     case MMToken::RSquare:
     case MMToken::RequiresKeyword:

lib/Lex/PPDirectives.cpp

 }
 
 const FileEntry *Preprocessor::LookupFile(
+    SourceLocation FilenameLoc,
     StringRef Filename,
     bool isAngled,
     const DirectoryLookup *FromDir,
     const DirectoryLookup *&CurDir,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
-    Module **SuggestedModule,
+    ModuleMap::KnownHeader *SuggestedModule,
     bool SkipCache) {
   // If the header lookup mechanism may be relative to the current file, pass in
   // info about where the current file is.

   const FileEntry *FE = HeaderInfo.LookupFile(
       Filename, isAngled, FromDir, CurDir, CurFileEnt,
       SearchPath, RelativePath, SuggestedModule, SkipCache);
-  if (FE) return FE;
+  if (FE) {
+    if (SuggestedModule) {
+      Module *RequestedModule = SuggestedModule->getModule();
+      if (RequestedModule) {
+        ModuleMap::ModuleHeaderRole Role = SuggestedModule->getRole();
+        #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
+            = RequestedModule->PrivateHeaders;
+        SmallVectorImpl<const FileEntry *>::iterator Look
+            = std::find(PvtHdrs.begin(), PvtHdrs.end(), FE);
+        bool IsPrivate = Look != PvtHdrs.end();
+        assert((IsPrivate && Role == ModuleMap::PrivateHeader)
+               || (!IsPrivate && Role != ModuleMap::PrivateHeader));
+        #endif
+        if (Role == ModuleMap::PrivateHeader) {
+          if (RequestedModule->getTopLevelModule() != getCurrentModule())
+            Diag(FilenameLoc, diag::error_use_of_private_header_outside_module)
+                 << Filename;
+        }
+      }
+    }
+    return FE;
+  }
 
   // Otherwise, see if this is a subframework header.  If so, this is relative
   // to one of the headers on the #include stack.  Walk the list of the current

   SmallString<1024> RelativePath;
   // We get the raw path only if we have 'Callbacks' to which we later pass
   // the path.
-  Module *SuggestedModule = 0;
+  ModuleMap::KnownHeader SuggestedModule;
+  SourceLocation FilenameLoc = FilenameTok.getLocation();
   const FileEntry *File = LookupFile(
-      Filename, isAngled, LookupFrom, CurDir,
+      FilenameLoc, Filename, isAngled, LookupFrom, CurDir,
       Callbacks ? &SearchPath : NULL, Callbacks ? &RelativePath : NULL,
       getLangOpts().Modules? &SuggestedModule : 0);
 

           HeaderInfo.AddSearchPath(DL, isAngled);
           
           // Try the lookup again, skipping the cache.
-          File = LookupFile(Filename, isAngled, LookupFrom, CurDir, 0, 0,
-                            getLangOpts().Modules? &SuggestedModule : 0,
+          File = LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, CurDir,
+                            0, 0, getLangOpts().Modules? &SuggestedModule : 0,
                             /*SkipCache*/true);
         }
       }

       // brackets, we can attempt a lookup as though it were a quoted path to
       // provide the user with a possible fixit.
       if (isAngled) {
-        File = LookupFile(Filename, false, LookupFrom, CurDir, 
+        File = LookupFile(FilenameLoc, Filename, false, LookupFrom, CurDir, 
                           Callbacks ? &SearchPath : 0, 
                           Callbacks ? &RelativePath : 0, 
                           getLangOpts().Modules ? &SuggestedModule : 0);

     // FIXME: Should we have a second loadModule() overload to avoid this
     // extra lookup step?
     SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Path;
-    for (Module *Mod = SuggestedModule; Mod; Mod = Mod->Parent)
+    for (Module *Mod = SuggestedModule.getModule(); Mod; Mod = Mod->Parent)
       Path.push_back(std::make_pair(getIdentifierInfo(Mod->Name),
                                     FilenameTok.getLocation()));
     std::reverse(Path.begin(), Path.end());

     ModuleLoadResult Imported
       = TheModuleLoader.loadModule(IncludeTok.getLocation(), Path, Visibility,
                                    /*IsIncludeDirective=*/true);
-    assert((Imported == 0 || Imported == SuggestedModule) &&
+    assert((Imported == 0 || Imported == SuggestedModule.getModule()) &&
            "the imported module is different than the suggested one");
     
     // If this header isn't part of the module we're building, we're done.

lib/Lex/PPMacroExpansion.cpp

   // Search include directories.
   const DirectoryLookup *CurDir;
   const FileEntry *File =
-      PP.LookupFile(Filename, isAngled, LookupFrom, CurDir, NULL, NULL, NULL);
+      PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, CurDir, NULL,
+                    NULL, NULL);
 
   // Get the result value.  A result of true means the file exists.
   return File != 0;

lib/Lex/Pragma.cpp

 
   // Search include directories for this file.
   const DirectoryLookup *CurDir;
-  const FileEntry *File = LookupFile(Filename, isAngled, 0, CurDir, NULL, NULL,
-                                     NULL);
+  const FileEntry *File = LookupFile(FilenameTok.getLocation(), Filename,
+                                     isAngled, 0, CurDir, NULL, NULL, NULL);
   if (File == 0) {
     if (!SuppressIncludeNotFoundError)
       Diag(FilenameTok, diag::err_pp_file_not_found) << Filename;

lib/Serialization/ASTReader.cpp

   using namespace clang::io;
   HeaderFileInfo HFI;
   unsigned Flags = *d++;
+  HFI.HeaderRole = static_cast<ModuleMap::ModuleHeaderRole>
+                   ((Flags >> 6) & 0x03);
   HFI.isImport = (Flags >> 5) & 0x01;
   HFI.isPragmaOnce = (Flags >> 4) & 0x01;
   HFI.DirInfo = (Flags >> 2) & 0x03;

       FileManager &FileMgr = Reader.getFileManager();
       ModuleMap &ModMap =
           Reader.getPreprocessor().getHeaderSearchInfo().getModuleMap();
-      ModMap.addHeader(Mod, FileMgr.getFile(key.Filename), /*Excluded=*/false);
+      ModMap.addHeader(Mod, FileMgr.getFile(key.Filename), HFI.getHeaderRole());
     }
   }
 

       if (First) {
         Error("missing submodule metadata record at beginning of block");
         return true;
+      }
+
+      if (!CurrentModule)
+        break;
+      
+      // 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_PRIVATE_HEADER: {
+      if (First) {
+        Error("missing submodule metadata record at beginning of block");
+        return true;
       }
 
       if (!CurrentModule)

lib/Serialization/ASTWriter.cpp

       using namespace clang::io;
       uint64_t Start = Out.tell(); (void)Start;
       
-      unsigned char Flags = (Data.isImport << 5)
+      unsigned char Flags = (Data.HeaderRole << 6)
+                          | (Data.isImport << 5)
                           | (Data.isPragmaOnce << 4)
                           | (Data.DirInfo << 2)
                           | (Data.Resolved << 1)

       Emit32(Out, Offset);
 
       if (Data.isModuleHeader) {
-        Module *Mod = HS.findModuleForHeader(key.FE);
+        Module *Mod = HS.findModuleForHeader(key.FE).getModule();
         Emit32(Out, Writer.getExistingSubmoduleID(Mod));
       }
 

   unsigned ExcludedHeaderAbbrev = Stream.EmitAbbrev(Abbrev);
 
   Abbrev = new BitCodeAbbrev();
+  Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_PRIVATE_HEADER));
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
+  unsigned PrivateHeaderAbbrev = Stream.EmitAbbrev(Abbrev);
+
+  Abbrev = new BitCodeAbbrev();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_LINK_LIBRARY));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));     // Name

     }
     
     // Emit the headers.
-    for (unsigned I = 0, N = Mod->Headers.size(); I != N; ++I) {
+    for (unsigned I = 0, N = Mod->NormalHeaders.size(); I != N; ++I) {
       Record.clear();
       Record.push_back(SUBMODULE_HEADER);
       Stream.EmitRecordWithBlob(HeaderAbbrev, Record, 
-                                Mod->Headers[I]->getName());
+                                Mod->NormalHeaders[I]->getName());
     }
     // Emit the excluded headers.
     for (unsigned I = 0, N = Mod->ExcludedHeaders.size(); I != N; ++I) {

       Stream.EmitRecordWithBlob(ExcludedHeaderAbbrev, Record, 
                                 Mod->ExcludedHeaders[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) {

test/Modules/Inputs/private/common.h

+#ifndef COMMON_H
+#define COMMON_H
+
+typedef int common;
+
+#endif

test/Modules/Inputs/private/module.map

+module libPrivate1 {
+  header "public1.h"
+  private header "private1.h"
+}
+
+module libPrivate2 {
+  header "public2.h"
+  private header "private2.h"
+}

test/Modules/Inputs/private/private1.h

+#ifndef PRIVATE1_H
+#define PRIVATE1_H
+
+#include "common.h"
+
+struct mitts_off1 { common field; };
+struct mitts_off1 hidden_variable1;
+
+#endif

test/Modules/Inputs/private/private2.h

+#ifndef PRIVATE2_H
+#define PRIVATE2_H
+
+#include "common.h"
+
+struct mitts_off2 { common field; };
+struct mitts_off2 hidden_variable2;
+
+#endif

test/Modules/Inputs/private/public1.h

+#ifndef PUBLIC1_H
+#define PUBLIC1_H
+
+#include "private1.h"
+
+struct use_this1 { struct mitts_off1 field; };
+struct use_this1 public_variable1;
+
+#endif

test/Modules/Inputs/private/public2.h

+#ifndef PUBLIC2_H
+#define PUBLIC2_H
+
+#include "private2.h"
+
+struct use_this2 { struct mitts_off2 field; };
+struct use_this2 public_variable2;
+
+#endif

test/Modules/Inputs/private0/common.h

+#ifndef COMMON_H
+#define COMMON_H
+
+typedef int common;
+
+#endif

test/Modules/Inputs/private1/module.map

+module libPrivate1 {
+  header "public1.h"
+  private header "private1.h"
+}

test/Modules/Inputs/private1/private1.h

+#ifndef PRIVATE1_H
+#define PRIVATE1_H
+
+#include "common.h"
+
+struct mitts_off1 { common field; };
+struct mitts_off1 hidden_variable1;
+
+#endif

test/Modules/Inputs/private1/public1.h

+#ifndef PUBLIC1_H
+#define PUBLIC1_H
+
+#include "private1.h"
+
+struct use_this1 { struct mitts_off1 field; };
+struct use_this1 public_variable1;
+
+#endif

test/Modules/Inputs/private2/module.map

+module libPrivate2 {
+  header "public2.h"
+  private header "private2.h"
+}

test/Modules/Inputs/private2/private2.h

+#ifndef PRIVATE2_H
+#define PRIVATE2_H
+
+#include "common.h"
+
+struct mitts_off2 { common field; };
+struct mitts_off2 hidden_variable2;
+
+#endif

test/Modules/Inputs/private2/public2.h

+#ifndef PUBLIC2_H
+#define PUBLIC2_H
+
+#include "private2.h"
+
+struct use_this2 { struct mitts_off2 field; };
+struct use_this2 public_variable2;
+
+#endif

test/Modules/private.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules-cache-path=%t -fmodules -I %S/Inputs/private %s -verify
+
+#include "common.h"
+@import libPrivate1;
+#include "private1.h" // expected-error {{use of private header from outside its module}}
+#include "public2.h"
+#include "private2.h" // expected-error {{use of private header from outside its module}}
+
+struct use_this1 client_variable1;
+struct use_this2 client_variable2;
+struct mitts_off1 client_variable3;
+struct mitts_off2 client_variable4;

test/Modules/private1.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules-cache-path=%t -fmodules -I %S/Inputs/private0 -I %S/Inputs/private1 -I %S/Inputs/private2 %s -verify
+
+#include "common.h"
+@import libPrivate1;
+#include "private1.h" // expected-error {{use of private header from outside its module}}
+#include "public2.h"
+#include "private2.h" // expected-error {{use of private header from outside its module}}
+
+struct use_this1 client_variable1;
+struct use_this2 client_variable2;
+struct mitts_off1 client_variable3;
+struct mitts_off2 client_variable4;