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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno closed this revision.
bruno added a comment.

Thanks for the review Richard.

Committed 291644




Comment at: lib/Lex/HeaderSearch.cpp:1082
 
+  auto TryEnterImported = [&](void) -> bool {
+if (!ModulesEnabled)

rsmith wrote:
> Maybe add a FIXME here indicating that this is a workaround for the lack of 
> proper modules-aware support for `#import` / `#pragma once`?
Done. 


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Lex/HeaderSearch.cpp:1082
 
+  auto TryEnterImported = [&](void) -> bool {
+if (!ModulesEnabled)

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


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@rsmith ping!


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 82067.
bruno marked 3 inline comments as done.
bruno added a comment.

Uploaded new patch with another approach to fix the issue.


https://reviews.llvm.org/D26267

Files:
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/ModuleMap.h
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  lib/Lex/PPDirectives.cpp
  test/Modules/Inputs/import-textual/M/A/A.h
  test/Modules/Inputs/import-textual/M/B/B.h
  test/Modules/Inputs/import-textual/M/module.modulemap
  test/Modules/Inputs/import-textual/M/someheader.h
  test/Modules/Inputs/import-textual/M2/A/A.h
  test/Modules/Inputs/import-textual/M2/B/B.h
  test/Modules/Inputs/import-textual/M2/module.modulemap
  test/Modules/Inputs/import-textual/M2/someheader.h
  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
  test/Modules/import-textual-noguard.mm
  test/Modules/import-textual.mm

Index: test/Modules/import-textual.mm
===
--- /dev/null
+++ test/Modules/import-textual.mm
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -fmodules -fimplicit-module-maps -I%S/Inputs/import-textual/M -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s -verify
+
+// expected-no-diagnostics
+
+#include "A/A.h"
+#include "B/B.h"
+
+typedef aint xxx;
+typedef bint yyy;
Index: test/Modules/import-textual-noguard.mm
===
--- /dev/null
+++ test/Modules/import-textual-noguard.mm
@@ -0,0 +1,8 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -fmodules -fimplicit-module-maps -I%S/Inputs/import-textual/M2 -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s -verify
+
+#include "A/A.h" // expected-error {{could not build module 'M'}}
+#include "B/B.h"
+
+typedef aint xxx;
+typedef bint yyy;
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 
+#include 
+#include 
+
+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
+++ 

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 3 inline comments as done.
bruno added a comment.



> 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.




Comment at: include/clang/Lex/HeaderSearch.h:98-101
+  /// 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.

rsmith wrote:
> 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?
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.



Comment at: lib/Lex/HeaderSearch.cpp:1116-1119
+//  - 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) {

rsmith wrote:
> 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.
Looks like what I actually wanted was to check if modules were supported at 
this point with `LangOpts.Modules`



Comment at: lib/Serialization/ASTReader.cpp:1691-1692
 HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+if (HFI.isImport)
+  HFI.addModulesUsingHdr(Mod);
   }

rsmith wrote:
> 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.
Ok


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

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.




Comment at: include/clang/Lex/HeaderSearch.h:98-101
+  /// 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.

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?



Comment at: lib/Lex/HeaderSearch.cpp:1116-1119
+//  - 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) {

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.



Comment at: lib/Serialization/ASTReader.cpp:1691-1692
 HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+if (HFI.isImport)
+  HFI.addModulesUsingHdr(Mod);
   }

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.


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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 

 col:12 in libc.math hidden 'int' -- FunctionDecl 0x7ffe23053a48 
 col:5 in libc.math hidden abs 'int (int)' `-ParmVarDecl 0x7ffe23053b20 
 col:12 in libc.math hidden 'int' -- TypedefDecl 0x7ffe23053f90 
 col:26 in libc.stddef hidden ptrdiff_t 'long' `-BuiltinType 
0x7ffe2300f9f0 'long' -- TypedefDecl 0x7ffe23054018 
 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 :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 
 col:23 in libc.stddef hidden size_t 'unsigned long'
  `-BuiltinType 0x7f81ed0ab890 'unsigned long'
  --
  TypedefDecl 0x7f81ed0f0938 
 col:23 in libc.stddef hidden rsize_t 'unsigned long'
  `-BuiltinType 0x7f81ed0ab890 'unsigned long'
  --
  TypedefDecl 0x7f81ed076778 
 col:26 in libc.stddef hidden ptrdiff_t 'long'
  `-BuiltinType 0x7f81ed03a9f0 'long'
  --
  TypedefDecl 0x7f81ed076800 
 col:15 in libc.POSIX.sys.types hidden ptrdiff_t 'int *'
  `-PointerType 0x7f81ed076850 'int *' imported
`-BuiltinType 0x7f81ed03a9d0 'int'
  --
  TypedefDecl 0x7f81ed0768b0 prev 0x7f81ed076778 
 col:26 in libc++.stddef hidden referenced ptrdiff_t 'long'
  `-BuiltinType 0x7f81ed03a9f0 'long'
  --
  TypedefDecl 0x7f81ed0769d0 
 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 
+#include 
+#include 
+
+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: 

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

[Richard Smith via cfe-commits]

On 9 December 2016 at 03:59, Richard Smith via Phabricator via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

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


Hmm, no, we inject that token immediately afterwards. I agree that the
module's contents should already be being made visible here. Can you look
at an AST dump and make sure that the declarations are owned by the correct
module? (You might need to try to use the declaration first, since we
populate the owning module information lazily.)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

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


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Any thoughts on this @rsmith ?


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> 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.


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via cfe-commits]

On 30 November 2016 at 11:08, Bruno Cardoso Lopes via Phabricator via
cfe-commits  wrote:

> > 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?


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. (Much like
we only skip an include due to an include guard if there is a visible
definition of the include guard macro.) For instance, instead of a single
`isImport` bit on `HeaderFileInfo`, we could store a list of IDs of
submodules that have imported the header, and check whether any of those
are visible when deciding whether to skip an inclusion, but there's
probably a cheaper way (in common cases) to get the same effect.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 
`//bin/../lib/clang/4.0.0/include/.h`, as 
`NormalHeader`, which maps to `HK_Normal` and (2) `.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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Frontend/FrontendActions.cpp:227
+IsBuiltin = true;
+  addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC,
+   IsBuiltin /*AlwaysInclude*/);

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.


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@rsmith ping!


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via cfe-commits]

bruno updated this revision to Diff 76931.
bruno marked an inline comment as done.
bruno added a comment.

Update patch after Vassil's comments!


https://reviews.llvm.org/D26267

Files:
  include/clang/Lex/ModuleMap.h
  lib/Frontend/FrontendActions.cpp
  lib/Lex/ModuleMap.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,6 @@
+// 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 
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 
+
+#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 
 
 #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 
 template T abs(T t) { return (t < 0) ? -t : t; }
 
+#include 
+
 #endif
Index: 

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

[Bruno Cardoso Lopes via cfe-commits]

bruno marked an inline comment as done.
bruno added a comment.

In https://reviews.llvm.org/D26267#586971, @v.g.vassilev wrote:

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


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




Comment at: include/clang/Lex/ModuleMap.h:278
+  /// headers.
+  static bool isBuiltinHeader(StringRef FileName);
+

v.g.vassilev wrote:
> It seems this is in the private section and it is accessed by 
> FrontendActions.cpp:224.
Oops!


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vassil Vassilev via cfe-commits]

v.g.vassilev added a comment.

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




Comment at: include/clang/Lex/ModuleMap.h:278
+  /// headers.
+  static bool isBuiltinHeader(StringRef FileName);
+

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


https://reviews.llvm.org/D26267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Cardoso Lopes via cfe-commits]

bruno created this revision.
bruno added a reviewer: rsmith.
bruno added subscribers: cfe-commits, manmanren.

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.


https://reviews.llvm.org/D26267

Files:
  include/clang/Lex/ModuleMap.h
  lib/Frontend/FrontendActions.cpp
  lib/Lex/ModuleMap.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,6 @@
+// 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 
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 
+
+#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 
 
 #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