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