[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?

1876–1893

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.

1876–1893

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

1876–1893

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

1876–1893

• 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

 
   /// \brief Whether the set of configuration macros is exhaustive.
   ///
   /// When the set of configuration macros is exhaustive, meaning
   /// that no identifier not in this list should affect how the module is
   /// built.
   unsigned ConfigMacrosExhaustive : 1;
 
+  /// \brief Whether this 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.
+  unsigned UsesRequiresExcludedHack : 1;
+
   /// \brief Describes the visibility of the various names within a
   /// particular module.
   enum NameVisibilityKind {
     /// \brief All of the names in this module are hidden.
     Hidden,
     /// \brief All of the names in this module are visible.
     AllVisible
   };

   struct Conflict {
     /// \brief The module that this module conflicts with.
     Module *Other;
 
     /// \brief The message provided to the user when there is a conflict.
     std::string Message;
   };
 
   /// \brief The list of conflicts.

lib/Basic/Module.cpp

 Module::Module(StringRef Name, SourceLocation DefinitionLoc, Module *Parent,
                bool IsFramework, bool IsExplicit, unsigned VisibilityID)
     : Name(Name), DefinitionLoc(DefinitionLoc), Parent(Parent), Directory(),
       Umbrella(), Signature(0), ASTFile(nullptr), VisibilityID(VisibilityID),
       IsMissingRequirement(false), IsAvailable(true), IsFromModuleFile(false),
       IsFramework(IsFramework), IsExplicit(IsExplicit), IsSystem(false),
       IsExternC(false), IsInferred(false), InferSubmodules(false),
       InferExplicitSubmodules(false), InferExportWildcard(false),
-      ConfigMacrosExhaustive(false), NameVisibility(Hidden) {
+      ConfigMacrosExhaustive(false), UsesRequiresExcludedHack(false),
+      NameVisibility(Hidden) {
   if (Parent) {
     if (!Parent->isAvailable())
       IsAvailable = false;
     if (Parent->IsSystem)
       IsSystem = true;
     if (Parent->IsExternC)
       IsExternC = true;
     IsMissingRequirement = Parent->IsMissingRequirement;
std::string Module::getFullModuleName() const {
   
   std::string Result;
   for (SmallVectorImpl<StringRef>::reverse_iterator I = Names.rbegin(),
                                                  IEnd = Names.rend();
        I != IEnd; ++I) {
     if (!Result.empty())
       Result += '.';
     
     Result += *I;

lib/Lex/ModuleMap.cpp

   if (const FileEntry *File = SourceMgr.getFileManager().getFile(FileNameRef))
     Map.parseModuleMapFile(
         File, /*IsSystem=*/false,
         Map.HeaderInfo.getHeaderSearchOpts().ModuleMapFileHomeIsCwd
             ? Directory
             : File->getDir(), ExternLoc);
 }
 
+static bool shouldAddRequirement(Module *M, StringRef Feature,
+                                 bool &IsRequiresExcludedHack) {
+  if (Feature == "excluded" || Feature == "cplusplus") {
+    std::string FullName = M->getFullModuleName();
+    if (FullName == "Darwin.C.excluded" || FullName == "Tcl.Private") {
+      // We will mark the module contents non-modular. See doc comment for
+      // Module::UsesRequiresExcludedHack.
+      IsRequiresExcludedHack = true;
+      return false;
+    } else if (FullName == "IOKit.avc") {
+      // This module was mistakenly marked 'requires cplusplus' in older Darwin
+      // SDK versions. As a backwards compatibility hack, don't add the
+      // requirement.
+      return false;
+    }
+  }
+
+  return true;
+}
+
 /// \brief Parse a requires declaration.
 ///
 ///   requires-declaration:
 ///     'requires' feature-list
 ///
 ///   feature-list:
 ///     feature ',' feature-list
 ///     feature
 ///
 ///   feature:
 ///     '!'[opt] identifier
 void ModuleMapParser::parseRequiresDecl() {
   assert(Tok.is(MMToken::RequiresKeyword));
 
   // Parse 'requires' keyword.
   consumeToken();
 
   // Parse the feature-list.
   do {
     bool RequiredState = true;
     if (Tok.is(MMToken::Exclaim)) {
       RequiredState = false;
       consumeToken();
     }
 
     if (!Tok.is(MMToken::Identifier)) {
       Diags.Report(Tok.getLocation(), diag::err_mmap_expected_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);
+    ActiveModule->UsesRequiresExcludedHack |= IsRequiresExcludedHack;
+
+    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 (ActiveModule->UsesRequiresExcludedHack) {
+    // 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;
     }

   }
   
   if (!Dir) {
     Diags.Report(DirNameLoc, diag::err_mmap_umbrella_dir_not_found)
       << DirName;
     HadError = true;
     return;
   }
-  
+  if (ActiveModule->UsesRequiresExcludedHack) {
+    // Mark this header 'textual' (see doc comment for
+    // Module::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;
+    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())) {
+        // FIXME: Taking the name from the FileEntry is unstable and can give
+        // different results depending on how we've previously named that file
+        // in this build.
+        Module::Header Header = {FE->getName(), FE};
+        Map.addHeader(ActiveModule, 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