bruno updated this revision to Diff 81478.
bruno added a comment.

Here is the Decl dump in the end of `ReadDeclRecord` for each Decl before it 
fails:

- ParmVarDecl 0x7ffe23053b20 
</Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/math.h:1:9>
 col:12 in libc.math hidden 'int' -- FunctionDecl 0x7ffe23053a48 
</Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/math.h:1:1,
 col:12> col:5 in libc.math hidden abs 'int (int)' `-ParmVarDecl 0x7ffe23053b20 
<col:9> col:12 in libc.math hidden 'int' -- TypedefDecl 0x7ffe23053f90 
</Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h:51:1, 
col:26> col:26 in libc.stddef hidden ptrdiff_t 'long' `-BuiltinType 
0x7ffe2300f9f0 'long' -- TypedefDecl 0x7ffe23054018 
</Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h:3:1,
 col:15> col:15 in libc.POSIX.sys.types hidden ptrdiff_t 'int *' `-PointerType 
0x7ffe23054070 'int *' imported `-BuiltinType 0x7ffe2300f9d0 'int' While 
building module 'libc++' imported from 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stdio.h:4:
 In file included from <module-includes>:1: In file included from 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h:7:
 In file included from 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits:4:
 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef:7:9:
 error: unknown type name 'ptrdiff_t' typedef ptrdiff_t my_ptrdiff_t; ^

So, both `libc.stddef` and `libc++.stddef`, answer for the builtin 
`/Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h`. 
`libc++.stddef` is being currently built, that's why the definition isn't there 
yet. Looks like the right thing is to indeed re-enter 
`/Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h`, so 
it can be put in `libc++.stddef`.

I've updated the patch to do what you suggested in the first place: track 
modules per header. Can you comment on the general approach? One consideration 
though; it's expensive to have the additional SmallVector per each 
HeaderFileInfo, since most headers (except builtins and textuals) aren't 
actually expected to have more than one associated Module. Maybe a map from 
HFIs to associated modules, but I'm not sure where to store it, suggestions? 
The Decl's loaded from the AST now make more sense (output resuming from the 
previously failing point):

  TypedefDecl 0x7f81ed0f0840 
</Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h:62:1, 
col:23> col:23 in libc.stddef hidden size_t 'unsigned long'
  `-BuiltinType 0x7f81ed0ab890 'unsigned long'
  --
  TypedefDecl 0x7f81ed0f0938 
</Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h:76:1, 
col:23> col:23 in libc.stddef hidden rsize_t 'unsigned long'
  `-BuiltinType 0x7f81ed0ab890 'unsigned long'
  --
  TypedefDecl 0x7f81ed076778 
</Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h:51:1, 
col:26> col:26 in libc.stddef hidden ptrdiff_t 'long'
  `-BuiltinType 0x7f81ed03a9f0 'long'
  --
  TypedefDecl 0x7f81ed076800 
</Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h:3:1,
 col:15> col:15 in libc.POSIX.sys.types hidden ptrdiff_t 'int *'
  `-PointerType 0x7f81ed076850 'int *' imported
    `-BuiltinType 0x7f81ed03a9d0 'int'
  --
  TypedefDecl 0x7f81ed0768b0 prev 0x7f81ed076778 
</Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h:51:1, 
col:26> col:26 in libc++.stddef hidden referenced ptrdiff_t 'long'
  `-BuiltinType 0x7f81ed03a9f0 'long'
  --
  TypedefDecl 0x7f81ed0769d0 
</Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef:7:1,
 col:19> col:19 in libc++.cstddef hidden my_ptrdiff_t 'ptrdiff_t':'long'
  `-TypedefType 0x7f81ed076920 'ptrdiff_t' sugar imported
    |-Typedef 0x7f81ed0768b0 'ptrdiff_t'
    `-BuiltinType 0x7f81ed03a9f0 'long'


https://reviews.llvm.org/D26267

Files:
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/Preprocessor.h
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
  test/Modules/builtin-import.mm

Index: test/Modules/builtin-import.mm
===================================================================
--- /dev/null
+++ test/Modules/builtin-import.mm
@@ -0,0 +1,12 @@
+// REQUIRES: system-darwin
+
+// RUN: rm -rf %t
+// RUN: %clang -cc1 -fsyntax-only -nostdinc++ -isysroot %S/Inputs/libc-libcxx/sysroot -isystem %S/Inputs/libc-libcxx/sysroot/usr/include/c++/v1 -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s
+
+#include <stdio.h>
+#include <stddef.h>
+#include <cstddef>
+
+typedef ptrdiff_t try1_ptrdiff_t;
+typedef my_ptrdiff_t try2_ptrdiff_t;
+
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
@@ -0,0 +1,6 @@
+#ifndef _SYS_TYPES_UMBRELLA
+#define _SYS_TYPES_UMBRELLA
+
+#include "_ptrdiff_t.h"
+
+#endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
@@ -0,0 +1,4 @@
+#ifndef _PTRDIFF_T
+#define _PTRDIFF_T
+typedef int * ptrdiff_t;
+#endif /* _PTRDIFF_T */
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
===================================================================
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
@@ -1 +1,6 @@
-// stddef.h
+#ifndef __STDDEF_H__
+#define __STDDEF_H__
+
+#include "sys/_types/_ptrdiff_t.h"
+
+#endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
===================================================================
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
@@ -5,4 +5,12 @@
   module stdint { header "stdint.h" export * }
   module stdio { header "stdio.h" export * }
   module util { header "util.h" export * }
+  module POSIX {
+    module sys {
+      module types {
+        umbrella header "sys/_types/_types.h"
+        export *
+      }
+    }
+  }
 }
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits
===================================================================
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits
@@ -0,0 +1,6 @@
+#ifndef _LIBCPP_TYPE_TRAITS
+#define _LIBCPP_TYPE_TRAITS
+
+#include <cstddef>
+
+#endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h
===================================================================
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h
@@ -2,5 +2,6 @@
 #define LIBCXX_STDDEF_H
 
 #include <__config>
+#include_next <stddef.h>
 
 #endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap
===================================================================
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap
@@ -6,5 +6,7 @@
   // FIXME: remove "textual" from stdint module below once the issue
   // between umbrella headers and builtins is resolved.
   module stdint { textual header "stdint.h" export * }
+  module type_traits { header "type_traits" export * }
+  module cstddef { header "cstddef" export * }
   module __config { header "__config" export * }
 }
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h
===================================================================
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h
@@ -4,4 +4,6 @@
 #include_next <math.h>
 template<typename T> T abs(T t) { return (t < 0) ? -t : t; }
 
+#include <type_traits>
+
 #endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef
===================================================================
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef
@@ -0,0 +1,9 @@
+#ifndef _LIBCPP_CSTDDEF
+#define _LIBCPP_CSTDDEF
+
+#include <stddef.h>
+#include <type_traits>
+
+typedef ptrdiff_t my_ptrdiff_t;
+
+#endif
Index: lib/Serialization/ASTReader.cpp
===================================================================
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -1688,6 +1688,8 @@
     Module::Header H = { key.Filename, FileMgr.getFile(Filename) };
     ModMap.addHeader(Mod, H, HeaderRole, /*Imported*/true);
     HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+    if (HFI.isImport)
+      HFI.addModulesUsingHdr(Mod);
   }
 
   // This HeaderFileInfo was externally loaded.
Index: lib/Lex/HeaderSearch.cpp
===================================================================
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -52,6 +52,27 @@
   return ControllingMacro;
 }
 
+void HeaderFileInfo::addModulesUsingHdr(const Module *M) {
+  for (auto *Mod : ModulesUsingHdr)
+    if (Mod == M)
+      return;
+  ModulesUsingHdr.push_back(M);
+}
+void HeaderFileInfo::addModulesUsingHdr(
+    ArrayRef<const Module *> OtherModulesUsingHdr) {
+  for (auto *M : OtherModulesUsingHdr)
+    addModulesUsingHdr(M);
+}
+ArrayRef<const Module *> HeaderFileInfo::getModulesUsingHdr() const {
+  return ModulesUsingHdr;
+}
+bool HeaderFileInfo::isModuleUsingHdr(const Module *M) {
+  for (auto *Mod : ModulesUsingHdr)
+    if (Mod == M)
+      return true;
+  return false;
+}
+
 ExternalHeaderFileInfoSource::~ExternalHeaderFileInfoSource() {}
 
 HeaderSearch::HeaderSearch(IntrusiveRefCntPtr<HeaderSearchOptions> HSOpts,
@@ -969,6 +990,8 @@
   HFI.isPragmaOnce |= OtherHFI.isPragmaOnce;
   HFI.isModuleHeader |= OtherHFI.isModuleHeader;
   HFI.NumIncludes += OtherHFI.NumIncludes;
+  if (OtherHFI.isImport || OtherHFI.isPragmaOnce)
+    HFI.addModulesUsingHdr(OtherHFI.getModulesUsingHdr());
 
   if (!HFI.ControllingMacro && !HFI.ControllingMacroID) {
     HFI.ControllingMacro = OtherHFI.ControllingMacro;
@@ -1089,9 +1112,18 @@
     if (FileInfo.NumIncludes) return false;
   } else {
     // Otherwise, if this is a #include of a file that was previously #import'd
-    // or if this is the second #include of a #pragma once file, ignore it.
-    if (FileInfo.isImport)
-      return false;
+    // or if this is the second #include of a #pragma once file, ignore it:
+    //  - if there not associated module or the module already imports
+    //    from this header.
+    //  - if any module associated with this header is visible.
+    if (FileInfo.isImport) {
+      if (!M || FileInfo.isModuleUsingHdr(M))
+        return false;
+      ArrayRef<const Module *> OtherMods = FileInfo.getModulesUsingHdr();
+      for (auto *OtherMod : OtherMods)
+        if (PP.CurSubmoduleState->VisibleModules.isVisible(OtherMod))
+          return false;
+    }
   }
 
   // Next, check to see if the file is wrapped with #ifndef guards.  If so, and
Index: include/clang/Lex/Preprocessor.h
===================================================================
--- include/clang/Lex/Preprocessor.h
+++ include/clang/Lex/Preprocessor.h
@@ -500,6 +500,7 @@
   typedef llvm::DenseMap<const IdentifierInfo *, MacroState> MacroMap;
 
   friend class ASTReader;
+  friend class HeaderSearch;
 
   struct SubmoduleState;
 
Index: include/clang/Lex/HeaderSearch.h
===================================================================
--- include/clang/Lex/HeaderSearch.h
+++ include/clang/Lex/HeaderSearch.h
@@ -95,6 +95,12 @@
   /// external storage.
   const IdentifierInfo *ControllingMacro;
 
+  /// List of modules that import from this header. Since the number of modules
+  /// using the same FileEntry is usually small (2 or 3 at most, often related
+  /// to builtins) this should be enough. Choose a more appropriate data
+  /// structure in case the requirement changes.
+  llvm::SmallVector<const Module *, 2> ModulesUsingHdr;
+
   /// \brief If this header came from a framework include, this is the name
   /// of the framework.
   StringRef Framework;
@@ -110,6 +116,15 @@
   const IdentifierInfo *
   getControllingMacro(ExternalPreprocessorSource *External);
 
+  /// \brief Add a module \p M that uses this header.
+  void addModulesUsingHdr(const Module *M);
+  /// \brief Add modules that uses this header from \p OtherModulesUsingHdr.
+  void addModulesUsingHdr(ArrayRef<const Module *> OtherModulesUsingHdr);
+  /// \return The list of modules importing from this header.
+  ArrayRef<const Module *> getModulesUsingHdr() const;
+  /// \return true if \p M is imports this header.
+  bool isModuleUsingHdr(const Module *M);
+
   /// \brief Determine whether this is a non-default header file info, e.g.,
   /// it corresponds to an actual header we've included or tried to include.
   bool isNonDefault() const {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to