[Modules] Be consistent about finding a module for framework headers

We use findModuleForHeader() in several places, but in header search we
were not calling it when a framework module didn't show up with the
expected name, which would then lead to unexpected non-modular includes.
Now we will find the module unconditionally for frameworks.  For regular
frameworks, we use the spelling of the module name from the module map
file, and for inferred ones we use the canonical directory name.

In the future we might want to lock down framework modules sufficiently
that these name mismatches cannot happen.

rdar://problem/20465870

llvm-svn: 241258

diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h
index 0bbcfac..2b18a7d 100644 (file)
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -231,8 +231,7 @@ private:
     return static_cast<bool>(findHeaderInUmbrellaDirs(File, IntermediateDirs));
   }
 
-  Module *inferFrameworkModule(StringRef ModuleName,
-                               const DirectoryEntry *FrameworkDir,
+  Module *inferFrameworkModule(const DirectoryEntry *FrameworkDir,
                                Attributes Attrs, Module *Parent);
 
 public:
@@ -344,10 +343,9 @@ public:
 
   /// \brief Infer the contents of a framework module map from the given
   /// framework directory.
-  Module *inferFrameworkModule(StringRef ModuleName, 
-                               const DirectoryEntry *FrameworkDir,
+  Module *inferFrameworkModule(const DirectoryEntry *FrameworkDir,
                                bool IsSystem, Module *Parent);
-  
+
   /// \brief Retrieve the module map file containing the definition of the given
   /// module.
   ///

diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 89120ff..6c5c64b 100644 (file)
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -532,9 +532,13 @@ const FileEntry *DirectoryLookup::DoFrameworkLookup(
       // Load this framework module. If that succeeds, find the suggested module
       // for this header, if any.
       bool IsSystem = getDirCharacteristic() != SrcMgr::C_User;
-      if (HS.loadFrameworkModule(ModuleName, TopFrameworkDir, IsSystem)) {
-        *SuggestedModule = HS.findModuleForHeader(FE);
-      }
+      HS.loadFrameworkModule(ModuleName, TopFrameworkDir, IsSystem);
+
+      // FIXME: This can find a module not part of ModuleName, which is
+      // important so that we're consistent about whether this header
+      // corresponds to a module. Possibly we should lock down framework modules
+      // so that this is not possible.
+      *SuggestedModule = HS.findModuleForHeader(FE);
     } else {
       *SuggestedModule = HS.findModuleForHeader(FE);
     }
@@ -1238,6 +1242,9 @@ Module *HeaderSearch::loadFrameworkModule(StringRef Name,
   // Try to load a module map file.
   switch (loadModuleMapFile(Dir, IsSystem, /*IsFramework*/true)) {
   case LMM_InvalidModuleMap:
+    // Try to infer a module map from the framework directory.
+    if (HSOpts->ImplicitModuleMaps)
+      ModMap.inferFrameworkModule(Dir, IsSystem, /*Parent=*/nullptr);
     break;
 
   case LMM_AlreadyLoaded:
@@ -1245,15 +1252,10 @@ Module *HeaderSearch::loadFrameworkModule(StringRef Name,
     return nullptr;
 
   case LMM_NewlyLoaded:
-    return ModMap.findModule(Name);
+    break;
   }
 
-
-  // Try to infer a module map from the framework directory.
-  if (HSOpts->ImplicitModuleMaps)
-    return ModMap.inferFrameworkModule(Name, Dir, IsSystem, /*Parent=*/nullptr);
-
-  return nullptr;
+  return ModMap.findModule(Name);
 }
 
 

diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 306401c..e6fe389 100644 (file)
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -583,19 +583,28 @@ static void inferFrameworkLink(Module *Mod, const DirectoryEntry *FrameworkDir,
   }
 }
 
-Module *
-ModuleMap::inferFrameworkModule(StringRef ModuleName,
-                                const DirectoryEntry *FrameworkDir,
-                                bool IsSystem,
-                                Module *Parent) {
+Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir,
+                                        bool IsSystem, Module *Parent) {
   Attributes Attrs;
   Attrs.IsSystem = IsSystem;
-  return inferFrameworkModule(ModuleName, FrameworkDir, Attrs, Parent);
+  return inferFrameworkModule(FrameworkDir, Attrs, Parent);
 }
 
-Module *ModuleMap::inferFrameworkModule(StringRef ModuleName,
-                                        const DirectoryEntry *FrameworkDir,
+Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir,
                                         Attributes Attrs, Module *Parent) {
+  // Note: as an egregious but useful hack we use the real path here, because
+  // we might be looking at an embedded framework that symlinks out to a
+  // top-level framework, and we need to infer as if we were naming the
+  // top-level framework.
+  StringRef FrameworkDirName =
+      SourceMgr.getFileManager().getCanonicalName(FrameworkDir);
+
+  // In case this is a case-insensitive filesystem, use the canonical
+  // directory name as the ModuleName, since modules are case-sensitive.
+  // FIXME: we should be able to give a fix-it hint for the correct spelling.
+  SmallString<32> ModuleNameStorage;
+  StringRef ModuleName = sanitizeFilenameAsIdentifier(
+      llvm::sys::path::stem(FrameworkDirName), ModuleNameStorage);
 
   // Check whether we've already found this module.
   if (Module *Mod = lookupModuleQualified(ModuleName, Parent))
@@ -608,20 +617,6 @@ Module *ModuleMap::inferFrameworkModule(StringRef ModuleName,
   const FileEntry *ModuleMapFile = nullptr;
   if (!Parent) {
     // Determine whether we're allowed to infer a module map.
-
-    // Note: as an egregious but useful hack we use the real path here, because
-    // we might be looking at an embedded framework that symlinks out to a
-    // top-level framework, and we need to infer as if we were naming the
-    // top-level framework.
-    StringRef FrameworkDirName
-      = SourceMgr.getFileManager().getCanonicalName(FrameworkDir);
-
-    // In case this is a case-insensitive filesystem, make sure the canonical
-    // directory name matches ModuleName exactly. Modules are case-sensitive.
-    // FIXME: we should be able to give a fix-it hint for the correct spelling.
-    if (llvm::sys::path::stem(FrameworkDirName) != ModuleName)
-      return nullptr;
-
     bool canInfer = false;
     if (llvm::sys::path::has_parent_path(FrameworkDirName)) {
       // Figure out the parent path.
@@ -747,10 +742,7 @@ Module *ModuleMap::inferFrameworkModule(StringRef ModuleName,
         continue;
 
       // FIXME: Do we want to warn about subframeworks without umbrella headers?
-      SmallString<32> NameBuf;
-      inferFrameworkModule(sanitizeFilenameAsIdentifier(
-                               llvm::sys::path::stem(Dir->path()), NameBuf),
-                           SubframeworkDir, Attrs, Result);
+      inferFrameworkModule(SubframeworkDir, Attrs, Result);
     }
   }
 

diff --git a/clang/test/Modules/Inputs/ImportNameInDir.h b/clang/test/Modules/Inputs/ImportNameInDir.h
new file mode 100644 (file)
index 0000000..ae7b1a0
--- /dev/null
+++ b/clang/test/Modules/Inputs/ImportNameInDir.h
@@ -0,0 +1,4 @@
+#import <NameInDir/NameInDir.h>
+
+// Don't crash.
+#undef NAME_IN_DIR

diff --git a/clang/test/Modules/Inputs/NameInDir.framework/Headers/NameInDir.h b/clang/test/Modules/Inputs/NameInDir.framework/Headers/NameInDir.h
new file mode 100644 (file)
index 0000000..bea2391
--- /dev/null
+++ b/clang/test/Modules/Inputs/NameInDir.framework/Headers/NameInDir.h
@@ -0,0 +1,2 @@
+// NameInDir.h
+#define NAME_IN_DIR 1

diff --git a/clang/test/Modules/Inputs/NameInDir.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/NameInDir.framework/Modules/module.modulemap
new file mode 100644 (file)
index 0000000..48e1c56
--- /dev/null
+++ b/clang/test/Modules/Inputs/NameInDir.framework/Modules/module.modulemap
@@ -0,0 +1,5 @@
+framework module NameInModMap {
+  umbrella header "NameInDir.h"
+  export *
+  module * { export * }
+}

diff --git a/clang/test/Modules/Inputs/NameInDir2.framework/Headers/NameInDir2.h b/clang/test/Modules/Inputs/NameInDir2.framework/Headers/NameInDir2.h
new file mode 100644 (file)
index 0000000..6dc3eea
--- /dev/null
+++ b/clang/test/Modules/Inputs/NameInDir2.framework/Headers/NameInDir2.h
@@ -0,0 +1 @@
+// NameInDir2.h

diff --git a/clang/test/Modules/Inputs/NameInDir2.framework/Modules/module.modulemap b/clang/test/Modules/Inputs/NameInDir2.framework/Modules/module.modulemap
new file mode 100644 (file)
index 0000000..24f15f8
--- /dev/null
+++ b/clang/test/Modules/Inputs/NameInDir2.framework/Modules/module.modulemap
@@ -0,0 +1,5 @@
+framework module NameInDir2 {
+  umbrella header "NameInDir2.h"
+  export *
+  module * { export * }
+}

diff --git a/clang/test/Modules/Inputs/NameInDirInferred.framework/Headers/NameInDirInferred.h b/clang/test/Modules/Inputs/NameInDirInferred.framework/Headers/NameInDirInferred.h
new file mode 100644 (file)
index 0000000..c0b12e6
--- /dev/null
+++ b/clang/test/Modules/Inputs/NameInDirInferred.framework/Headers/NameInDirInferred.h
@@ -0,0 +1 @@
+// NameInDirInferred.h

diff --git a/clang/test/Modules/Inputs/module.map b/clang/test/Modules/Inputs/module.map
index b9c8e72..ffaa53e 100644 (file)
--- a/clang/test/Modules/Inputs/module.map
+++ b/clang/test/Modules/Inputs/module.map
@@ -332,3 +332,7 @@ module DebugModule {
   header "DebugModule.h"
 }
 
+module ImportNameInDir {
+  header "ImportNameInDir.h"
+  export *
+}

diff --git a/clang/test/Modules/framework-name.m b/clang/test/Modules/framework-name.m
new file mode 100644 (file)
index 0000000..a63e206
--- /dev/null
+++ b/clang/test/Modules/framework-name.m
@@ -0,0 +1,33 @@
+// REQUIRES: shell
+// RUN: rm -rf %t.mcp %t
+// RUN: mkdir -p %t
+// RUN: ln -s %S/Inputs/NameInDir2.framework %t/NameInImport.framework
+// RUN: ln -s %S/Inputs/NameInDirInferred.framework %t/NameInImportInferred.framework
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.mcp -fimplicit-module-maps -I %S/Inputs -F %S/Inputs -F %t -Wauto-import -verify %s
+
+// Sanity check that we won't somehow find non-canonical module names or
+// modules where we shouldn't search the framework.
+// RUN: echo '@import NameInModMap' | not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.mcp -F %S/Inputs -F %t -Wauto-import -x objective-c - 2>&1 | FileCheck %s
+// RUN: echo '@import NameInDir' | not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.mcp -F %S/Inputs -F %t -Wauto-import -x objective-c - 2>&1 | FileCheck %s
+// RUN: echo '@import NameInImport' | not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.mcp -F %S/Inputs -F %t -Wauto-import -x objective-c - 2>&1 | FileCheck %s
+// RUN: echo '@import NameInImportInferred' | not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.mcp -F %S/Inputs -F %t -Wauto-import -x objective-c - 2>&1 | FileCheck %s
+// CHECK: module '{{.*}}' not found
+
+// FIXME: We might want to someday lock down framework modules so that these
+// name mismatches are disallowed. However, as long as we *don't* prevent them
+// it's important that they map correctly to module imports.
+
+// The module map name doesn't match the directory name.
+#import <NameInDir/NameInDir.h> // expected-warning {{import of module 'NameInModMap'}}
+
+// The name in the import doesn't match the module name.
+#import <NameInImport/NameInDir2.h> // expected-warning {{import of module 'NameInDir2'}}
+@import NameInDir2;                 // OK
+
+// The name in the import doesn't match the module name (inferred framework module).
+#import <NameInImportInferred/NameInDirInferred.h> // expected-warning {{import of module 'NameInDirInferred'}}
+
+@import ImportNameInDir;
+#ifdef NAME_IN_DIR
+#error NAME_IN_DIR should be undef'd
+#endif