<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 14, 2016 at 11:44 AM, Bruno Cardoso Lopes <span dir="ltr"><<a href="mailto:bruno.cardoso@gmail.com" target="_blank">bruno.cardoso@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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></blockquote><div><br></div><div><div>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.<br></div><div><br></div><div>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). That exposed a couple of testcases that were (unreasonably, in my opinion) failing to include_next the real builtin header from their wrapper header.</div><div><br></div><div>The attached patch causes your testcase to pass; I'd be interested to know if it works in practice on Darwin.</div><div><br></div><div>Thanks!</div><div> <br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Can you take a look?<br>
<br>
Thanks,<br>
<br>
<br>
On Thu, Jul 28, 2016 at 3:55 PM, Adrian Prantl via cfe-commits<br>
<div class="m_3301338866323896559gmail-HOEnZb"><div class="m_3301338866323896559gmail-h5"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
> +Bruno<br>
><br>
> On Jul 27, 2016, at 11:58 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
><br>
> I played with modules a bit today, and as far as I can tell this is still<br>
> broken. If this proves difficult to fix, should this change be reverted for<br>
> now? It breaks using modules on Darwin.<br>
><br>
> On Sun, Mar 13, 2016 at 12:53 AM, Adrian Prantl via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> > On Mar 11, 2016, at 4:11 PM, Duncan P. N. Exon Smith<br>
>> > <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>> ><br>
>> > Did anyone file a PR for this?<br>
>> ><br>
>><br>
>> I filed PR 26928 - Prune the include path for modules<br>
>> <a href="https://llvm.org/bugs/show_bug.cgi?id=26928" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug<wbr>.cgi?id=26928</a><br>
>> as a starting point.<br>
>><br>
>> -- adrian<br>
>><br>
>> ______________________________<wbr>_________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">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>
><br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">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>
<br>
<br>
<br>
</div></div><span class="m_3301338866323896559gmail-HOEnZb"><font color="#888888">--<br>
Bruno Cardoso Lopes<br>
<a href="http://www.brunocardoso.cc" rel="noreferrer" target="_blank">http://www.brunocardoso.cc</a><br>
</font></span></blockquote></div><br></div></div>