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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.


================
Comment at: lib/Frontend/FrontendActions.cpp:227
+        IsBuiltin = true;
+      addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC,
+                       IsBuiltin /*AlwaysInclude*/);
----------------
rsmith wrote:
> 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.
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?


https://reviews.llvm.org/D26267