[Modules] Add Darwin-specific compatibility module map parsing hacks

[benlangmuir]

Summary

I ended up restricting the "requires excluded" hack to the specifically named modules after all. It turned out to only affect the two modules I had found: Darwin.C.excluded and Tcl.Private. I tested this back to OS X 10.8 and iOS 7.

This preserves backwards compatibility for two hacks in the Darwin
 system module map files:
 
 1. The use of 'requires excluded' to make headers non-modular, which
 should really be mapped to 'textual' now that we have this feature.
 
 2. Silently removes a bogus cplusplus requirement from IOKit.avc.
 
 Once we start diagnosing missing requirements and headers on
 auto-imports these would have broken compatibility with existing Darwin
 SDKs.

[silvas]

My gut is that this seems too complicated, but on the other hand I don't have a suggestion for simplifying it. I'll let Richard decide on what is an acceptable level of complexity.

Could you put a lengthy comment somewhere describing exactly which shipping module map files are affected by this, and the range of affected SDK versions (hopefully this will be fixed in the next release?)?

[silvas]

Ping.

[benlangmuir]

Sorry for the slow response, I somehow missed your original comment.

Add a more cohesive comment. I don't think we can usefully specify the SDK range it affects until a fixed SDK is available (currently it's every SDK that has module maps at all). I'd love to update the comment then. Ideally at that point we could also check the SDK version before applying the hack, but I'm not sure it's worth reading the SDK version plist file from disk just for that. We already read this file when creating the module cache hash, but I'd like to remove that at some point.

[rsmith]

include/clang/Basic/Module.h

203–211

Do we really need to write this into the Module object? Could we instead just track this while we're parsing the module map?

lib/Lex/ModuleMap.cpp

1590–1603

Please handle the excluded and cplusplus cases separately, to keep this special case as narrow as possible.

1591

Seems like overkill to compute the full module name for every module that requires cplusplus. Instead, how about walking the module path a component at a time and checking you get the expected sequence of components?

1891–1908

Is there a better way of handling this? If the parent directory isn't itself an umbrella directory of some module, maybe you could just ignore the umbrella directory declaration for this module entirely?

[benlangmuir]

include/clang/Basic/Module.h

203–211

Will do.

lib/Lex/ModuleMap.cpp

1590–1603

Will do. Not sure why I ever combined them like that...

1591

Makes sense, will do.

1891–1908

This only affects Tcl.Private, and Tcl has an umbrella header so I don't think that will work. The only other way I can think of making this work is to have a notion of a *directory* that is exempt from its containing umbrella, but I'm not sure that's a generally good feature and it seems like a lot more work. Let me know if you have any suggestions though.

[rsmith]

lib/Lex/ModuleMap.cpp

1891–1908

Ugh, OK. In that case:

• maybe take the file name from I->path() rather than FE->getName() (we want to imagine the files were named within the umbrella directory rather than some other way)
• sort the paths before you add them so that the serialized pcm doesn't depend on file system traversal order

Also, you'll be paying this file system traversal cost whenever the relevant module map is parsed, not only when the Tcl module is used -- and if it's the /usr/include module map, you'll walk this umbrella directory on every build. Just wanted to confirm you're OK with that.

[benlangmuir]

lib/Lex/ModuleMap.cpp

1891–1908

• maybe take the file name from I->path() rather than FE->getName()
• sort the paths

Ah, good catches!

Also, you'll be paying this file system traversal cost whenever the relevant module map is parsed,

Tcl is a framework so not affected by looking at usr/include. But your comment made me double check where else we might parse this. I think the answer is:

1. When Tcl is named in an import or header include. This seems fine.
2. When we call collectAllModules()...

I'm not happy about (2) since it can happen during code completion of imports. I suppose we're already iterating directories and parsing a tonne of other module map files here. I'll benchmark this and see how awful it is...

[benlangmuir]

Changes per review. I still need to check performance of code-completing import statements to make sure the extra directory iteration isn't a big regression.

[rsmith]

Looks fine, go ahead once you're satisfied that the performance is OK for the requires excluded / umbrella dir hack.

include/clang/Basic/Module.h

363

fullModuleNameEquals (extra 's' at the end) or fullModuleNameIs maybe?

lib/Basic/Module.cpp

142–147

Seems weird to do this by modifying nameParts rather than tracking an index, but OK...

lib/Lex/ModuleMap.cpp

1025–1026

Unnecessary newline?

1607–1608

Overindented.

[benlangmuir]

Any regression to code completion was below the noise level in my testing.

r244912

include/clang/Basic/Module.h

363

I like fullModuleNameIs.

include/clang/Basic/Module.h

   bool isSubFramework() const {
     return IsFramework && Parent && Parent->isPartOfFramework();
   }
 
   /// \brief Retrieve the full name of this module, including the path from
   /// its top-level module.
   std::string getFullModuleName() const;
 
+  /// \brief Whether the full name of this module is equal to joining
+  /// \p nameParts with "."s.
+  ///
+  /// This is more efficient than getFullModuleName().
+  bool fullModuleNameEqual(ArrayRef<StringRef> nameParts) const;
+
   /// \brief Retrieve the top-level module for this (sub)module, which may
   /// be this module.
   Module *getTopLevelModule() {
     return const_cast<Module *>(
              const_cast<const Module *>(this)->getTopLevelModule());
   }
 
   /// \brief Retrieve the top-level module for this (sub)module, which may

lib/Basic/Module.cpp

       Result += '.';
     
     Result += *I;
   }
   
   return Result;
 }
 
+bool Module::fullModuleNameEqual(ArrayRef<StringRef> nameParts) const {
+  for (const Module *M = this; M; M = M->Parent) {
+    if (nameParts.empty() || M->Name != nameParts.back())
+      return false;
+    nameParts = nameParts.drop_back();
+  }
+  return nameParts.empty();
+}
+
 Module::DirectoryName Module::getUmbrellaDir() const {
   if (Header U = getUmbrellaHeader())
     return {"", U.Entry->getDir()};
   
   return {UmbrellaAsWritten, Umbrella.dyn_cast<const DirectoryEntry *>()};
 }
 
 ArrayRef<const FileEntry *> Module::getTopHeaders(FileManager &FileMgr) {

lib/Lex/ModuleMap.cpp

     /// during parsing.
     llvm::BumpPtrAllocator StringData;
     
     /// \brief The current token.
     MMToken Tok;
     
     /// \brief The active module.
     Module *ActiveModule;
-    
+    /// \brief Whether a module uses the 'requires excluded' hack to mark its
+    /// contents as 'textual'.
+    ///
+    /// On older Darwin SDK versions, 'requires excluded' is used to mark the
+    /// contents of the Darwin.C.excluded (assert.h) and Tcl.Private modules as
+    /// non-modular headers.  For backwards compatibility, we continue to
+    /// support
+    /// this idiom for just these modules, and map the headers to 'textual' to
+    /// match the original intent.
+    llvm::SmallPtrSet<Module *, 2> UsesRequiresExcludedHack;
+
     /// \brief Consume the current token and return its location.
     SourceLocation consumeToken();
     
     /// \brief Skip tokens until we reach the a token with the given kind
     /// (or the end of the file).
     void skipUntil(MMToken::TokenKind K);
 
     typedef SmallVector<std::pair<std::string, SourceLocation>, 2> ModuleId;

   if (const FileEntry *File = SourceMgr.getFileManager().getFile(FileNameRef))
     Map.parseModuleMapFile(
         File, /*IsSystem=*/false,
         Map.HeaderInfo.getHeaderSearchOpts().ModuleMapFileHomeIsCwd
             ? Directory
             : File->getDir(), ExternLoc);
 }
 
+/// Whether to add the requirement \p Feature to the module \p M.
+///
+/// This preserves backwards compatibility for two hacks in the Darwin system
+/// module map files:
+///
+/// 1. The use of 'requires excluded' to make headers non-modular, which
+///    should really be mapped to 'textual' now that we have this feature.  We
+///    drop the 'excluded' requirement, and set \p IsRequiresExcludedHack to
+///    true.  Later, this bit will be used to map all the headers inside this
+///    module to 'textual'.
+///
+///    This affects Darwin.C.excluded (for assert.h) and Tcl.Private.
+///
+/// 2. Removes a bogus cplusplus requirement from IOKit.avc.  This requirement
+///    was never correct and causes issues now that we check it, so drop it.
+static bool shouldAddRequirement(Module *M, StringRef Feature,
+                                 bool &IsRequiresExcludedHack) {
+  static const StringRef DarwinCExcluded[] = {"Darwin", "C", "excluded"};
+  static const StringRef TclPrivate[] = {"Tcl", "Private"};
+  static const StringRef IOKitAVC[] = {"IOKit", "avc"};
+
+  if (Feature == "excluded" && (M->fullModuleNameEqual(DarwinCExcluded) ||
+                                M->fullModuleNameEqual(TclPrivate))) {
+      IsRequiresExcludedHack = true;
+      return false;
+  } else if (Feature == "cplusplus" && M->fullModuleNameEqual(IOKitAVC)) {
+    return false;
+  }
+
+  return true;
+}
+
 /// \brief Parse a requires declaration.
 ///
 ///   requires-declaration:
 ///     'requires' feature-list
 ///
 ///   feature-list:
 ///     feature ',' feature-list
 ///     feature

       HadError = true;
       return;
     }
 
     // Consume the feature name.
     std::string Feature = Tok.getString();
     consumeToken();
 
-    // Add this feature.
-    ActiveModule->addRequirement(Feature, RequiredState,
-                                 Map.LangOpts, *Map.Target);
+    bool IsRequiresExcludedHack = false;
+    bool ShouldAddRequirement =
+        shouldAddRequirement(ActiveModule, Feature, IsRequiresExcludedHack);
+
+    if (IsRequiresExcludedHack)
+      UsesRequiresExcludedHack.insert(ActiveModule);
+
+    if (ShouldAddRequirement) {
+      // Add this feature.
+      ActiveModule->addRequirement(Feature, RequiredState, Map.LangOpts,
+                                   *Map.Target);
+    }
 
     if (!Tok.is(MMToken::Comma))
       break;
 
     // Consume the comma.
     consumeToken();
   } while (true);
 }

   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 (UsesRequiresExcludedHack.count(ActiveModule)) {
+    // Mark this header 'textual' (see doc comment for
+    // Module::UsesRequiresExcludedHack).
+    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;
     }

   // Look for this file.
   const DirectoryEntry *Dir = nullptr;
   if (llvm::sys::path::is_absolute(DirName))
     Dir = SourceMgr.getFileManager().getDirectory(DirName);
   else {
     SmallString<128> PathName;
     PathName = Directory->getName();
     llvm::sys::path::append(PathName, DirName);
     Dir = SourceMgr.getFileManager().getDirectory(PathName);
   }
   
   if (!Dir) {
     Diags.Report(DirNameLoc, diag::err_mmap_umbrella_dir_not_found)
       << DirName;
     HadError = true;
     return;
   }
-  
+  if (UsesRequiresExcludedHack.count(ActiveModule)) {
+    // Mark this header 'textual' (see doc comment for
+    // ModuleMapParser::UsesRequiresExcludedHack). Although iterating over the
+    // directory is relatively expensive, in practice this only applies to the
+    // uncommonly used Tcl module on Darwin platforms.
+    std::error_code EC;
+    SmallVector<Module::Header, 6> Headers;
+    for (llvm::sys::fs::recursive_directory_iterator I(Dir->getName(), EC), E;
+         I != E && !EC; I.increment(EC)) {
+      if (const FileEntry *FE = SourceMgr.getFileManager().getFile(I->path())) {
+
+        Module::Header Header = {I->path(), FE};
+        Headers.push_back(std::move(Header));
+      }
+    }
+
+    // Sort header paths so that the pcm doesn't depend on iteration order.
+    llvm::array_pod_sort(Headers.begin(), Headers.end(),
+                         [](const Module::Header *A, const Module::Header *B) {
+                           return A->NameAsWritten.compare(B->NameAsWritten);
+                         });
+    for (auto &Header : Headers)
+      Map.addHeader(ActiveModule, std::move(Header), ModuleMap::TextualHeader);
+    return;
+  }
+
   if (Module *OwningModule = Map.UmbrellaDirs[Dir]) {
     Diags.Report(UmbrellaLoc, diag::err_mmap_umbrella_clash)
       << OwningModule->getFullModuleName();
     HadError = true;
     return;
-  } 
-  
+  }
   // Record this umbrella directory.
   Map.setUmbrellaDir(ActiveModule, Dir, DirName);
 }
 
 /// \brief Parse a module export declaration.
 ///
 ///   export-declaration:
 ///     'export' wildcard-module-id

test/Modules/Inputs/System/usr/include/assert.h

+// assert.h
+#define DARWIN_C_EXCLUDED 1

test/Modules/Inputs/System/usr/include/module.map

explicit module dbl_max {
     header "dbl_max.h"
   }
 }
 
 module uses_other_constants {
   header "uses_other_constants.h"
   export *
 }
+
+module Darwin {
+  module C {
+    module excluded {
+      requires excluded
+      header "assert.h"
+    }
+  }
+}
+
+module Tcl {
+  module Private {
+    requires excluded
+    umbrella ""
+  }
+}
+
+module IOKit {
+  module avc {
+    requires cplusplus
+  }
+}

test/Modules/Inputs/System/usr/include/tcl-private/header.h

+// tcl-private/header.h
+#define TCL_PRIVATE 1

test/Modules/darwin_specific_modulemap_hacks.m

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fimplicit-module-maps -isystem %S/Inputs/System/usr/include -triple x86_64-apple-darwin10 %s -verify -fsyntax-only
+// expected-no-diagnostics
+
+@import Darwin.C.excluded; // no error, header is implicitly 'textual'
+@import Tcl.Private;       // no error, header is implicitly 'textual'
+@import IOKit.avc;         // no error, cplusplus requirement removed
+
+#if defined(DARWIN_C_EXCLUDED)
+#error assert.h should be textual
+#elif defined(TCL_PRIVATE)
+#error tcl-private/header.h should be textual
+#endif
+
+#import <assert.h>
+#import <tcl-private/header.h>
+
+#if !defined(DARWIN_C_EXCLUDED)
+#error assert.h missing
+#elif !defined(TCL_PRIVATE)
+#error tcl-private/header.h missing
+#endif