On Mon, Oct 17, 2016 at 3:31 PM, Manman via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
>
> > 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.
>
I don't think we did (see the FIXME I deleted suggesting to use the local
submodule visibiltiy logic in all cases).
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
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
