[Modules] Include builtins with #include instead of #import for ObjC

[bruno]

Summary

After r284797 builins are treated like textual includes. When compiling for ObjC++, the in-memory header file generated for the modules is composed only of #import's instead of includes. That could block some textual includes to happen because #import's will not re-enter headers already handled by other #import's.

Use #include when the header in question is a built-in.

[v.g.vassilev]

Could you include more context when creating the diff eg. git diff -U9999, or equivalent.

include/clang/Lex/ModuleMap.h

278

It seems this is in the private section and it is accessed by FrontendActions.cpp:224.

[bruno]

In D26267#586971, @v.g.vassilev wrote:

Could you include more context when creating the diff eg. git diff -U9999, or equivalent.

I did, -U99999 actually, not sure why you're not getting it...

include/clang/Lex/ModuleMap.h

278

Oops!

[bruno]

Update patch after Vassil's comments!

[bruno]

Ping!

[bruno]

@rsmith ping!

[rsmith]

lib/Frontend/FrontendActions.cpp

227

I don't understand why this would be necessary. If these headers are in fact textual headers, they won't be in the HK_Normal or HK_Private lists at all (they'll be in the HK_Private or HK_PrivateTextual lists instead), so your IsBuiltin = true; line should be unreachable. (Checking for an absolute path also seems suspicious here.)

I suspect the problem is instead that #import just doesn't work properly with modules (specifically, either with local submodule visibility or with modules that do not export *), because it doesn't care whether the previous inclusion is visible or not, and as a result it assumes "I've heard about someone #including this ever" means the same thing as "the contents of this file are visible right now". I know that #pragma once has this issue, so I'd expect #import to also suffer from it.

[bruno]

lib/Frontend/FrontendActions.cpp

227

In fact, I can achieve the same desired result by changing libcxx modulemap to:

@@ -161,11 +161,15 @@ module std [system] {
     module cstddef {
       header "cstddef"
       export *
+      // FIXME: #import on Darwin requires explicit re-export
+      export Darwin.POSIX.sys.types
     }
     module cstdint {
       header "cstdint"
       export depr.stdint_h
       export *
+      // FIXME: #import on Darwin requires explicit re-export
+      export Darwin.C.stdint
     }

But I would like to avoid adding darwin specific stuff there.

I don't understand why this would be necessary. If these headers are in fact textual headers,
they won't be in the HK_Normal or HK_Private lists at all (they'll be in the HK_Private or
HK_PrivateTextual lists instead), so your IsBuiltin = true; line should be unreachable.

AFAIU, builtins are added twice (1) with the full path with /<path_to_clang_install>/bin/../lib/clang/4.0.0/include/<builtin>.h, as NormalHeader, which maps to HK_Normal and (2) <builtin>.h as TextualHeader, mapping to HK_Textual. This happens in the snippet (lib/Lex/ModuleMap.cpp:1882) below:

if (BuiltinFile) {
  // 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 H = { BuiltinFile->getName(), BuiltinFile };
  Map.addHeader(ActiveModule, H, Role); <- (1)

  // If we have both a builtin and system version of the file, the
  // builtin version may want to inject macros into the system header, so
  // force the system header to be treated as a textual header in this
  // case.
  Role = ModuleMap::ModuleHeaderRole(Role | ModuleMap::TextualHeader);
}

// Record this header.
Module::Header H = { RelativePathName.str(), File };
Map.addHeader(ActiveModule, H, Role); <- (2)

As an experiment, I changed collectModuleHeaderIncludes to skip addHeaderInclude for builtin headers with HK_Normal, but it caused regressions while compiling Darwin SDK even for non local submodule visibility.

(Checking for an absolute path also seems suspicious here.)

Right, this was done to speedup checking for built-ins, since they are expanded to absolute paths. But this isn't necessary for the logic to work and can be removed.

I suspect the problem is instead that #import just doesn't work properly with modules
(specifically, either with local submodule visibility or with modules that do not export *),
because it doesn't care whether the previous inclusion is visible or not, and as a result it assumes
"I've heard about someone #including this ever" means the same thing as "the contents of this file
are visible right now". I know that #pragma once has this issue, so I'd expect #import to also suffer from it.

Taking a look at this, any ideas on what's the right approach with the #import's here? So instead of entering the header, is there a way to make the contents for the module matching the built-in header to become available/visible at that point?

[bruno]

The right thing to do would be to track the set of headers whose #import / #include-with-#pragma-once is
visible, and only skip inclusions if there is a *visible* #import / #include-with-#pragma-once of that header.

This makes sense, but the situation is a bit more weird with the builtins: at the time clang skips entering /Users/bruno/Dev/srcs/llvm/debug/bin/../lib/clang/4.0.0/include/stddef.h because there's already a #import associated with it, clang tries to get the module, in this case it finds "libc++.stddef", but makeModuleVisible does not export the contents
of /Users/bruno/Dev/srcs/llvm/debug/bin/../lib/clang/4.0.0/include/stddef.h:

// If we don't need to enter the file, stop now.
if (!ShouldEnter) {
  // If this is a module import, make it visible if needed.
  if (auto *M = SuggestedModule.getModule()) {
    makeModuleVisible(M, HashLoc);
    ...
  }
  return;
}

(lldb) p M->dump()
module stddef   [system] {
  header "/Users/bruno/Dev/srcs/llvm/debug/bin/../lib/clang/4.0.0/include/stddef.h"
  textual header "stddef.h"
  export *
}

Is this what it's supposed to happen with a (textual) builtin header? It seems to me that since the first #import was done after collecting the headers to build the module, the content should already been present/available in the AST for the module.

[bruno]

Any thoughts on this @rsmith ?

[rsmith]

Looks like we might only be making macros visible, and failing to emit the annot_module_import token for Sema.

[bruno]

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'

[rsmith]

Other options:

Do we need to perfectly preserve the functionality of #pragma once / #import? That is, would it be acceptable to you if we spuriously enter files that have been imported in that way, while building a module? If we view them as pure optimization, we can do something substantially simpler here, for instance by never importing "isImported" flags from modules and throwing away all our "isImported" flags whenever module visibility decreases.

Another possibility -- albeit slightly hacky -- would be to invent a controlling macro if the header in question turns out to not have one (say, form an identifier from the path by which the file was included and use that as the macro name) and is #imported / uses #pragma once. Or we could try simply rejecting textual headers that are #import'd / #pragma once'd and have no controlling macro in a modules build.

include/clang/Lex/HeaderSearch.h

98–101 ↗

(On Diff #81478)

I don't think this is true in general. For a textual header, there could be hundreds or thousands of loaded modules that use it. If all header files in a project start with

#include "local_config.h"

... which is configured to be a textual header for whatever reason and contains a #pragma once, we would get into that situation for at least that one header. While a vector might be OK (as the number of entries should at least grow only linearly with the size of the entire codebase), quadratic-time algorithms over it probably won't be. Perhaps a sorted vector instead?

lib/Lex/HeaderSearch.cpp

1116–1119 ↗

(On Diff #81478)

I think this would be clearer as:

[...], ignore it, unless doing so will lead to us importing a module that contains the file but didn't actually include it (because we're still building the corresponding module), and we've not already made the file visible by importing some other module.

... but I don't think that's actually right. If M is null (if the file is only ever included as a textual header), we still need to check whether some visible module has actually made it visible, and we should enter it if not.

lib/Serialization/ASTReader.cpp

1691–1692 ↗

(On Diff #81478)

Doing this here will do the wrong thing while building a module with local submodule visibility enabled -- we need to know whether the visible module set contains a module that entered the header, even for modules that we've never serialized.

Also, this will not populate the vector for the cases where the header was not listed in the module map, but was simply a non-moduler header that was textually entered while building a module.

[bruno]

Do we need to perfectly preserve the functionality of #pragma once / #import? That is, would it be acceptable to
you if we spuriously enter files that have been imported in that way, while building a module?

Unfortunately we can find ObjC headers that don't have include guards and only rely on #import's. Relaxing this condition could lead to more redefinition errors with current code base than we desire.

Another possibility -- albeit slightly hacky -- would be to invent a controlling macro if the header in question turns out to not have one (say, form an identifier from the path by which the file was included and use that as the macro name) and is #imported / uses #pragma once. Or we could try simply rejecting textual headers that are #import'd / #pragma once'd and have no controlling macro in a modules build.

Right, it's not that hacky - it actually performs the job of actually guarding as we do with includes, but with more context. However, it's not clear to me how to determine the suggested 'identifier from the path'.

I have a third suggestion: seems like we only care here about two cases (1) modular import's from the same header - this will only happens with builtins AFAIU and (2) textual headers that must be imported multiple times for different modules. We can solve (1) by only using the information on HFI and fix (2) by only re-trying to enter the #imports where we find a controlling macro for the header and that macro isn't reachable at that point. I wrote a new patch that implements this, let me know what are your thoughts. Note that I intentionally left non-modular includes out because AFAIK they should be part of the first module they are founded, so it doesn't make any sense to re-enter them anyway.

include/clang/Lex/HeaderSearch.h

98–101 ↗

(On Diff #81478)

Right, I wasn't considering adding the textual headers at this point, this should have been filtered out by the ASTReader HFI merging. But handling the textual headers is necessary indeed. Hopefully my next patch fix it.

lib/Lex/HeaderSearch.cpp

1116–1119 ↗

(On Diff #81478)

Looks like what I actually wanted was to check if modules were supported at this point with LangOpts.Modules

lib/Serialization/ASTReader.cpp

1691–1692 ↗

(On Diff #81478)

Ok

[bruno]

Uploaded new patch with another approach to fix the issue.

[bruno]

Ping!

[bruno]

@rsmith ping!

[rsmith]

lib/Lex/HeaderSearch.cpp

1082 ↗

(On Diff #82067)

Maybe add a FIXME here indicating that this is a workaround for the lack of proper modules-aware support for #import / #pragma once?

[bruno]

Thanks for the review Richard.

Committed 291644

lib/Lex/HeaderSearch.cpp

1082 ↗

(On Diff #82067)

Done.

include/clang/Lex/ModuleMap.h

private:
 
   /// \brief Given that \p File is not in the Headers map, look it up within
   /// umbrella directories and find or create a module for it.
   KnownHeader findOrCreateModuleForHeaderInUmbrellaDir(const FileEntry *File);
 
   /// \brief A convenience method to determine if \p File is (possibly nested)
   /// in an umbrella directory.
   bool isHeaderInUmbrellaDirs(const FileEntry *File) {
     SmallVector<const DirectoryEntry *, 2> IntermediateDirs;
     return static_cast<bool>(findHeaderInUmbrellaDirs(File, IntermediateDirs));
   }
 
   Module *inferFrameworkModule(const DirectoryEntry *FrameworkDir,
                                Attributes Attrs, Module *Parent);
 
 public:
   /// \brief Construct a new module map.
public:
   void setTarget(const TargetInfo &Target);
 
   /// \brief Set the directory that contains Clang-supplied include
   /// files, such as our stdarg.h or tgmath.h.
   void setBuiltinIncludeDir(const DirectoryEntry *Dir) {
     BuiltinIncludeDir = Dir;
   }
 
+  /// \brief Determine whether the given file name is the name of a builtin
+  /// header, supplied by Clang to replace, override, or augment existing system
+  /// headers.
+  static bool isBuiltinHeader(StringRef FileName);
+
   /// \brief Add a module map callback.
   void addModuleMapCallbacks(std::unique_ptr<ModuleMapCallbacks> Callback) {
     Callbacks.push_back(std::move(Callback));
   }
 
   /// \brief Retrieve the module that owns the given header file, if any.
   ///
   /// \param File The header file that is likely to be included.

lib/Frontend/FrontendActions.cpp

 operator+=(SmallVectorImpl<char> &Includes, StringRef RHS) {
   Includes.append(RHS.begin(), RHS.end());
   return Includes;
 }
 
 static void addHeaderInclude(StringRef HeaderName,
                              SmallVectorImpl<char> &Includes,
                              const LangOptions &LangOpts,
-                             bool IsExternC) {
+                             bool IsExternC, bool AlwaysInclude = false) {
   if (IsExternC && LangOpts.CPlusPlus)
     Includes += "extern \"C\" {\n";
-  if (LangOpts.ObjC1)
+  if (LangOpts.ObjC1 && !AlwaysInclude)
     Includes += "#import \"";
   else
     Includes += "#include \"";
 
   Includes += HeaderName;
 
   Includes += "\"\n";
   if (IsExternC && LangOpts.CPlusPlus)
collectModuleHeaderIncludes(const LangOptions &LangOpts, FileManager &FileMgr,
   // Add includes for each of these headers.
   for (auto HK : {Module::HK_Normal, Module::HK_Private}) {
     for (Module::Header &H : Module->Headers[HK]) {
       Module->addTopHeader(H.Entry);
       // Use the path as specified in the module map file. We'll look for this
       // file relative to the module build directory (the directory containing
       // the module map file) so this will find the same file that we found
       // while parsing the module map.
-      addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC);
+      // Since builtins are treated as textual headers and there's no context
+      // leak in local submodule visibility, make sure we use #include
+      // instead of #import (even in ObjC/C++) and guarantee it's always
+      // included.
+      bool IsBuiltin = false;
+      if (LangOpts.ObjC1 && llvm::sys::path::is_absolute(H.NameAsWritten) &&
+          ModuleMap::isBuiltinHeader(
+              llvm::sys::path::filename(H.NameAsWritten)))
+        IsBuiltin = true;
+      addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC,
+                       IsBuiltin /*AlwaysInclude*/);
     }
   }
   // Note that Module->PrivateHeaders will not be a TopHeader.
 
   if (Module::Header UmbrellaHeader = Module->getUmbrellaHeader()) {
     Module->addTopHeader(UmbrellaHeader.Entry);
     if (Module->Parent)
       // Include the umbrella header for submodules.

lib/Lex/ModuleMap.cpp

#include "clang/Basic/TokenKinds.def"
   }
 
   return Name;
 }
 
 /// \brief Determine whether the given file name is the name of a builtin
 /// header, supplied by Clang to replace, override, or augment existing system
 /// headers.
-static bool isBuiltinHeader(StringRef FileName) {
+bool ModuleMap::isBuiltinHeader(StringRef FileName) {
   return llvm::StringSwitch<bool>(FileName)
            .Case("float.h", true)
            .Case("iso646.h", true)
            .Case("limits.h", true)
            .Case("stdalign.h", true)
            .Case("stdarg.h", true)
            .Case("stdatomic.h", true)
            .Case("stdbool.h", true)
if (ActiveModule->isPartOfFramework()) {
       llvm::sys::path::append(FullPathName, RelativePathName);
       File = SourceMgr.getFileManager().getFile(FullPathName);
 
       // If this is a system module with a top-level header, this header
       // may have a counterpart (or replacement) in the set of headers
       // supplied by Clang. Find that builtin header.
       if (ActiveModule->IsSystem && LeadingToken != MMToken::UmbrellaKeyword &&
           BuiltinIncludeDir && BuiltinIncludeDir != Directory &&
-          isBuiltinHeader(Header.FileName)) {
+          ModuleMap::isBuiltinHeader(Header.FileName)) {
         SmallString<128> BuiltinPathName(BuiltinIncludeDir->getName());
         llvm::sys::path::append(BuiltinPathName, Header.FileName);
         BuiltinFile = SourceMgr.getFileManager().getFile(BuiltinPathName);
 
         // If Clang supplies this header but the underlying system does not,
         // just silently swap in our builtin version. Otherwise, we'll end
         // up adding both (later).
         if (BuiltinFile && !File) {

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef

+#ifndef _LIBCPP_CSTDDEF
+#define _LIBCPP_CSTDDEF
+
+#include <stddef.h>
+#include <type_traits>
+
+typedef ptrdiff_t my_ptrdiff_t;
+
+#endif

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h

 #ifndef LIBCXX_MATH_H
 #define LIBCXX_MATH_H
 
 #include_next <math.h>
 template<typename T> T abs(T t) { return (t < 0) ? -t : t; }
 
+#include <type_traits>
+
 #endif

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap

 module "libc++" {
   module math { header "math.h" export * }
   module stdlib { header "stdlib.h" export * }
   module stddef { header "stddef.h" export * }
   module stdio { textual header "stdio.h" export * }
   // 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 * }
 }

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h

 #ifndef LIBCXX_STDDEF_H
 #define LIBCXX_STDDEF_H
 
 #include <__config>
+#include_next <stddef.h>
 
 #endif

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits

+#ifndef _LIBCPP_TYPE_TRAITS
+#define _LIBCPP_TYPE_TRAITS
+
+#include <cstddef>
+
+#endif

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap

 module libc [no_undeclared_includes] {
   module math { header "math.h" export * }
   module stdlib { header "stdlib.h" export * }
   module stddef { header "stddef.h" export * }
   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 *
+      }
+    }
+  }
 }

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h

-// stddef.h
+#ifndef __STDDEF_H__
+#define __STDDEF_H__
+
+#include "sys/_types/_ptrdiff_t.h"
+
+#endif

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h

+#ifndef _PTRDIFF_T
+#define _PTRDIFF_T
+typedef int * ptrdiff_t;
+#endif /* _PTRDIFF_T */

test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h

+#ifndef _SYS_TYPES_UMBRELLA
+#define _SYS_TYPES_UMBRELLA
+
+#include "_ptrdiff_t.h"
+
+#endif

test/Modules/builtin-import.mm

+// 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 -F%S/Inputs/libc-libcxx/sysroot/Frameworks -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s
+
+#include <stdio.h>