[modules] Attempt to fix PR31905 - #include "stddef.h" breaks module map search paths; causes redefinitions.

[EricWF]

Summary

This patch attempts to fix llvm.org/PR31905.

The fix is to only allow the compiler to search the top-level include directories when looking a module.modulemap definition in ASTReader.cpp. This prevent loading arbitrary module maps from directories that would never normally be searched for a module map for _Builtin_stddef_max_align_t.

Perhaps it would be better to figure out what top-level search directory the module was originally built under, and then remove the prefix of the top-level directory. Then we could allow the search to look under matching subdirectories of other top-level search paths. However this seems like it could be incorrect as well.

[EricWF]

I've got a better, actually correct, solution incoming.

[EricWF]

• Only allow the top level header search directories to be search for module.modulemap files when loading modules in ReadAST.cpp.

Perhaps it would be better to figure out what top-level search directory the module was originally built under, and then remove the prefix of the top-level directory. Then we could allow the search to look under matching subdirectories of other top-level search paths. However this seems like it could be incorrect as well.

@zygoloid: Any thoughts?

[rsmith]

lib/Serialization/ASTReader.cpp

2554–2556

This is only trying to catch the case where we've already loaded a module map for the module and the two conflict. We should just set AllowSearch to false here. If we want to also perform validation on the case where we load the module map after importing another definition, that should be handled if/when we load the module map.

include/clang/Lex/HeaderSearch.h

public:
   /// \brief Lookup a module Search for a module with the given name.
   ///
   /// \param ModuleName The name of the module we're looking for.
   ///
   /// \param AllowSearch Whether we are allowed to search in the various
   /// search directories to produce a module definition. If not, this lookup
   /// will only return an already-known module.
   ///
+  /// \param SearchTopLevelOnly Whether we are allowed to search the
+  /// subdirectories to produce a module definition.
+  ///
   /// \returns The module with the given name.
-  Module *lookupModule(StringRef ModuleName, bool AllowSearch = true);
+  Module *lookupModule(StringRef ModuleName, bool AllowSearch = true,
+                       bool SearchTopLevelOnly = false);
 
   /// \brief Try to find a module map file in the given directory, returning
   /// \c nullptr if none is found.
   const FileEntry *lookupModuleMapFile(const DirectoryEntry *Dir,
                                        bool IsFramework);
   
   void IncrementFrameworkLookupCount() { ++NumFrameworkLookups; }
 
public:
   ///
   /// \param Modules Will be filled with the set of known, top-level modules.
   void collectAllModules(SmallVectorImpl<Module *> &Modules);
 
   /// \brief Load all known, top-level system modules.
   void loadTopLevelSystemModules();
 
 private:
-
   /// \brief Lookup a module with the given module name and search-name.
   ///
   /// \param ModuleName The name of the module we're looking for.
   ///
   /// \param SearchName The "search-name" to derive filesystem paths from
   /// when looking for the module map; this is usually equal to ModuleName,
   /// but for compatibility with some buggy frameworks, additional attempts
   /// may be made to find the module under a related-but-different search-name.
   ///
+  /// \param SearchTopLevelOnly Whether we are allowed to search the
+  /// subdirectories to produce a module definition.
+  ///
   /// \returns The module named ModuleName.
-  Module *lookupModule(StringRef ModuleName, StringRef SearchName);
+  Module *lookupModule(StringRef ModuleName, StringRef SearchName,
+                       bool SearchTopLevelOnly);
 
   /// \brief Retrieve a module with the given name, which may be part of the
   /// given framework.
   ///
   /// \param Name The name of the module to retrieve.
   ///
   /// \param Dir The framework directory (e.g., ModuleName.framework).
   ///

lib/Lex/HeaderSearch.cpp

if (HSOpts->DisableModuleHash) {
 
     SmallString<128> HashStr;
     llvm::APInt(64, size_t(Hash)).toStringUnsigned(HashStr, /*Radix*/36);
     llvm::sys::path::append(Result, ModuleName + "-" + HashStr + ".pcm");
   }
   return Result.str().str();
 }
 
-Module *HeaderSearch::lookupModule(StringRef ModuleName, bool AllowSearch) {
+Module *HeaderSearch::lookupModule(StringRef ModuleName, bool AllowSearch,
+                                   bool SearchTopLevelOnly) {
   // Look in the module map to determine if there is a module by this name.
   Module *Module = ModMap.findModule(ModuleName);
   if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps)
     return Module;
 
   StringRef SearchName = ModuleName;
-  Module = lookupModule(ModuleName, SearchName);
+  Module = lookupModule(ModuleName, SearchName, SearchTopLevelOnly);
 
   // The facility for "private modules" -- adjacent, optional module maps named
   // module.private.modulemap that are supposed to define private submodules --
   // is sometimes misused by frameworks that name their associated private
   // module FooPrivate, rather than as a submodule named Foo.Private as
   // intended. Here we compensate for such cases by looking in directories named
   // Foo.framework, when we previously looked and failed to find a
   // FooPrivate.framework.
   if (!Module && SearchName.consume_back("Private"))
-    Module = lookupModule(ModuleName, SearchName);
+    Module = lookupModule(ModuleName, SearchName, SearchTopLevelOnly);
   return Module;
 }
 
-Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) {
+Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName,
+                                   bool SearchTopLevelOnly) {
   Module *Module = nullptr;
 
   // Look through the various header search paths to load any available module
   // maps, searching for a module map that describes this module.
   for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
     if (SearchDirs[Idx].isFramework()) {
       // Search for or infer a module map for a framework. Here we use
       // SearchName rather than ModuleName, to permit finding private modules
if (loadModuleMapFile(NestedModuleMapDirName, IsSystem,
       // If we just loaded a module map file, look for the module again.
       Module = ModMap.findModule(ModuleName);
       if (Module)
         break;
     }
 
     // If we've already performed the exhaustive search for module maps in this
     // search directory, don't do it again.
-    if (SearchDirs[Idx].haveSearchedAllModuleMaps())
+    if (SearchTopLevelOnly || SearchDirs[Idx].haveSearchedAllModuleMaps())
       continue;
 
     // Load all module maps in the immediate subdirectories of this search
     // directory.
     loadSubdirectoryModuleMaps(SearchDirs[Idx]);
 
     // Look again for the module.
     Module = ModMap.findModule(ModuleName);

lib/Serialization/ASTReader.cpp

 
       break;
 
     case MODULE_DIRECTORY: {
       assert(!F.ModuleName.empty() &&
              "MODULE_DIRECTORY found before MODULE_NAME");
       // If we've already loaded a module map file covering this module, we may
       // have a better path for it (relative to the current build).
-      Module *M = PP.getHeaderSearchInfo().lookupModule(F.ModuleName);
+      Module *M = PP.getHeaderSearchInfo().lookupModule(
+          F.ModuleName,
+          /*AllowSearch*/ true, /*SearchTopLevelOnly*/ true);
       if (M && M->Directory) {
         // If we're implicitly loading a module, the base directory can't
         // change between the build and use.
         if (F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) {
           const DirectoryEntry *BuildDir =
               PP.getFileManager().getDirectory(Blob);
           if (!BuildDir || BuildDir != M->Directory) {
             if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)

test/Modules/Inputs/PR31905/my-project/module.modulemap

+module my_project {
+  header "my-header.h"
+}

test/Modules/Inputs/PR31905/my-project/my-header.h

+#ifndef MY_HEADER_H
+#define MY_HEADER_H
+#include <stddef.h>
+#endif

test/Modules/pr31905.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cp -R %S/Inputs/PR31905 %t/other-include
+// RUN: %clang_cc1 -std=c++11 -I%S/Inputs/PR31905/ -I%t/other-include -fmodules -fmodules-local-submodule-visibility \
+// RUN:    -fimplicit-module-maps -fmodules-cache-path=%t -verify %s
+
+#include "my-project/my-header.h"
+
+int main() {} // expected-no-diagnostics