[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: Sean Silva <chisophugis () gmail ! com>
Date: 2015-07-09 22:47:57
Message-ID: CAHnXoak70TrDQ3P7daDzsHh6K18-QRHt4JEnaMjF4NPPk8w+hw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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. Fixing this (probably not
with my simple patch attached here) seems like a good change, but unrelated
to this patch.
-- 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"><br><div class="gmail_extra"><br><div class="gmail_quote">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 class=""><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 class=""><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><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. Fixing this (probably not with my simple patch attached here) seems \
like a good change, but unrelated to this patch.</div><div><br></div><div>-- Sean \
Silva</div><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 class=""><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=JAbRF \
gmrUKN1pO2tI03AKWq6bDcfnv8w0NLeVtoIa14&s=vUAU_0L3gYQdFlYVNtYFm3EVtOazMXvzZa2wWwZxyCA&e=" \
rel="noreferrer" target="_blank">http://reviews.llvm.org/D10423</a><br> <br>
<br>
<br>
</blockquote></div><br></div></div>
--001a1133048af35d02051a790b59--
["markUnavailable.patch" (application/octet-stream)]
diff --git a/lib/Basic/Module.cpp b/lib/Basic/Module.cpp
index 1a48a6c..9eb135f 100644
--- a/lib/Basic/Module.cpp
+++ b/lib/Basic/Module.cpp
@@ -184,18 +184,12 @@ void Module::addRequirement(StringRef Feature, bool RequiredState,
}
void Module::markUnavailable(bool MissingRequirement) {
- if (!IsAvailable)
- return;
-
SmallVector<Module *, 2> Stack;
Stack.push_back(this);
while (!Stack.empty()) {
Module *Current = Stack.back();
Stack.pop_back();
- if (!Current->IsAvailable)
- continue;
-
Current->IsAvailable = false;
Current->IsMissingRequirement |= MissingRequirement;
for (submodule_iterator Sub = Current->submodule_begin(),
_______________________________________________
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