> On Oct 17, 2016, at 2:11 PM, Bruno Cardoso Lopes via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > Hi, > > On Fri, Oct 14, 2016 at 3:09 PM, Richard Smith <rich...@metafoo.co.uk> wrote: >> On Fri, Oct 14, 2016 at 11:44 AM, Bruno Cardoso Lopes >> <bruno.card...@gmail.com> wrote: >>> >>> Hi Richard, >>> >>> I have a patch on top of your suggested patch from a year ago, that >>> break the cyclic dependency we're seeing, with this (and a few changes >>> to the SDK) we can bootstrap clang with submodule local visibility on >>> darwin. I've attached the patch with a reduced, standalone testcase >>> that doesn't depend on the SDK. The issues that are not covered by >>> your patch, that I cover in mine, are related to built-in and textual >>> headers: they can be found in paths where they don't map to any >>> modules, triggering other cycles. I fix that by looking further to >>> find a matching module for the header in question, instead the first >>> found header in header search. >> >> >> It looks like the 0002 patch is working around a bug in the 0001 patch: with >> 0001 applied, a module with [no_undeclared_includes] can still include a >> textual header from another module on which it doesn't declare a dependency >> (in the testcase, the libc module is incorrectly permitted to include the >> textual header <stdio.h> from libc++). Rather than preferring non-modular >> headers over modular headers as the 0002 patch does, I wonder if the issue >> could instead be resolved by fixing that apparent bug. > > Thanks for the fast answer and for the new patch :-) > My intend with 0002 was actually to prefer modular headers instead of > non-modular, but fallback to the later in case none is found. > >> I gave that a go; the result is attached. I also had to change the way we >> handle builtin headers -- instead of implicitly including (for instance) the >> builtin <stddef.h> as a modular header in any module that provides its own >> <stddef.h>, I now only include it as a textual header (this also lets us use >> the same approach for this case whether or not we're using local submodule >> visibility).
@Richard, I wonder why (prior to your patch) we need to use a different approach for builtin headers with local submodule visibility. Thanks, >> That exposed a couple of testcases that were (unreasonably, in >> my opinion) failing to include_next the real builtin header from their >> wrapper header. > > I'm curious about these, are they from clang tests? > >> The attached patch causes your testcase to pass; I'd be interested to know >> if it works in practice on Darwin. > > It works for building the Darwin module, but failed for "#include > <algorithm>" on darwin (which was working under 0001+0002 patch): > > While building module 'std' imported from all-headers.cpp:1: > While building module 'Darwin' imported from > /clang-install/include/c++/v1/string.h:61: > In file included from <module-includes>:422: > In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach.h:67: > In file included from > /SDK/MacOSX10.12.sdk/usr/include/mach/mach_interface.h:42: > In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/clock_priv.h:6: > /clang-install/include/c++/v1/string.h:74:7: error: redefinition of > '__libcpp_strchr' > char* __libcpp_strchr(const char* __s, int __c) {return > (char*)strchr(__s, __c);} > ^ > /clang-install/include/c++/v1/string.h:74:7: note: previous definition is here @Bruno, Can you try "-fdiagnostics-show-note-include-stackā so we know the other path that leads to string.h? Manman > char* __libcpp_strchr(const char* __s, int __c) {return > (char*)strchr(__s, __c);} > ^ > While building module 'std' imported from all-headers.cpp:1: > While building module 'Darwin' imported from > /clang-install/include/c++/v1/string.h:61: > In file included from <module-includes>:422: > In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/mach.h:67: > In file included from > /SDK/MacOSX10.12.sdk/usr/include/mach/mach_interface.h:42: > In file included from /SDK/MacOSX10.12.sdk/usr/include/mach/clock_priv.h:6: > /clang-install/include/c++/v1/string.h:76:13: error: redefinition of 'strchr' > const char* strchr(const char* __s, int __c) {return __libcpp_strchr(__s, > __c);} > ^ > /clang-install/include/c++/v1/string.h:76:13: note: previous definition is > here > const char* strchr(const char* __s, int __c) {return __libcpp_strchr(__s, > __c);} > ^ > <...> > While building module 'std' imported from all-headers.cpp:1: > In file included from <module-includes>:1: > In file included from /clang-install/include/c++/v1/algorithm:638: > In file included from /clang-install/include/c++/v1/cstring:61: > /clang-install/include/c++/v1/string.h:61:15: fatal error: could not > build module 'Darwin' > #include_next <string.h> > ~~~~~~~~~~~~~^ > all-headers.cpp:1:10: fatal error: could not build module 'std' > #include <algorithm> > ~~~~~~~~^ > 17 errors generated. > > It looks like "#include_next <string.h>" is poiting back to > /clang-install/include/c++/v1/string.h > FWIW, string.h is also in SDK/usr/include/string.h, under > Darwin.C.string module. > I'll get back to you with a small testcase once I'm able to reduce this. > > Thanks! > > > -- > Bruno Cardoso Lopes > http://www.brunocardoso.cc > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits