[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D10423: [modules] PR20507: Avoid silent textual inclusion.
From: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-09 23:06:38
Message-ID: CAOfiQq=d4R5PW9Sq5AagA4pP5K+-kkzAhiB05z3eW5tMozgKtg () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Thu, Jul 9, 2015 at 3:47 PM, Sean Silva <chisophugis@gmail.com> wrote:
>
>
> On Wed, Jul 8, 2015 at 12:58 PM, Ben Langmuir <blangmuir@apple.com> wrote:
>
>> benlangmuir added a comment.
>>
>> > - Added better source location.
>>
>>
>> It should really be the requirement location, but I guess we don't store
>> that anywhere. This can be improved separately.
>>
>> > - Updated testing. I renamed to auto-import-unavailable.cpp and
>> expanded testing as you asked for. Before, I was piggybacking on
>> missing-header.m and related files, but since that is a different code path
>> (and has different requirements and diagnostics), I thought it was better
>> to keep the test separate and self-contained.
>>
>> > - also emit note_implicit_top_level_module_import_here on missing
>> requirement.
>>
>>
>> Changes look good, thanks.
>>
>> I replied to your email about the issue when a "requires" clause comes
>> *after* a header declaration. I didn't actually hit that problem in the
>> wild - only in a synthetic test - so maybe it can be addressed
>> separately. If you and Richard agree, then from my perspective your patch
>> LGTM and we just need to get the Darwin hacks in before it lands.
>>
>
> This seems orthogonal. It is due to Module::markUnavailable having
> multiple bailouts that are incorrect if MissingRequirement would have been
> changed on any of the submodules during the traversal. The attached patch
> fixes that, but I think opens us up to O(n^2) behavior.
>
Can we fix that by changing the bailout condition to:
if (MissingRequirement ? !IsAvailable : IsMissingRequirement)
?
Fixing this (probably not with my simple patch attached here) seems like a
> good change, but unrelated to this patch.
>
Am I right in thinking that your patch causes regressions on valid input
without a fix to this issue?
> -- Sean Silva
>
>
>>
>> > Ben: hopefully the present patch is complete enough for you to start
>>
>> > figuring out exactly what hacks to add and where.
>>
>>
>> Definitely. I can work from this, thanks.
>>
>>
>> http://reviews.llvm.org/D10423
>>
>>
>>
>>
>
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jul 9, 2015 \
at 3:47 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" \
target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div \
class="gmail_quote"><span class="">On Wed, Jul 8, 2015 at 12:58 PM, Ben Langmuir \
<span dir="ltr"><<a href="mailto:blangmuir@apple.com" \
target="_blank">blangmuir@apple.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">benlangmuir \
added a comment.<br> <span><br>
> - Added better source location.<br>
<br>
<br>
</span>It should really be the requirement location, but I guess we don't store \
that anywhere. This can be improved separately.<br> <span><br>
> - Updated testing. I renamed to auto-import-unavailable.cpp and expanded testing \
as you asked for. Before, I was piggybacking on missing-header.m and related files, \
but since that is a different code path (and has different requirements and \
diagnostics), I thought it was better to keep the test separate and \
self-contained.<br> <br>
> - also emit note_implicit_top_level_module_import_here on missing \
requirement.<br> <br>
<br>
</span>Changes look good, thanks.<br>
<br>
I replied to your email about the issue when a "requires" clause comes \
*after* a header declaration. I didn't actually hit that problem in the wild - \
only in a synthetic test - so maybe it can be addressed separately. If you and \
Richard agree, then from my perspective your patch LGTM and we just need to get the \
Darwin hacks in before it lands.<br></blockquote><div><br></div></span><div>This \
seems orthogonal. It is due to Module::markUnavailable having multiple bailouts that \
are incorrect if MissingRequirement would have been changed on any of the submodules \
during the traversal. The attached patch fixes that, but I think opens us up to \
O(n^2) behavior.</div></div></div></div></blockquote><div><br></div><div>Can we fix \
that by changing the bailout condition to:</div><div><br></div><div> if \
(MissingRequirement ? !IsAvailable : IsMissingRequirement)</div><div> \
<br></div><div>?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 \
0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote"><div>Fixing this (probably not with my \
simple patch attached here) seems like a good change, but unrelated to this \
patch.</div></div></div></div></blockquote><div><br></div><div>Am I right in thinking \
that your patch causes regressions on valid input without a fix to this \
issue?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font \
color="#888888"><div>-- Sean Silva</div></font></span><span class=""><div> \
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> Ben: hopefully the present patch is complete enough for you to start<br>
<br>
> figuring out exactly what hacks to add and where.<br>
<br>
<br>
</span>Definitely. I can work from this, thanks.<br>
<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10423&d \
=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=gDn2h \
lH35Yf-KDAe-6SvZBrvsBv7VK3UrrJciTWznOs&s=wIP4cY4_2OB0FDLTo0VSBqN7EjnYX46Hw4WE9DTD-W0&e=" \
rel="noreferrer" target="_blank">http://reviews.llvm.org/D10423</a><br> <br>
<br>
<br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic