[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-13 21:09:51
Message-ID: CAOfiQq=OKYWpGJAQTNM7H__k08xEzHPEUmte70gqeAu8+B-=zw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Fri, Jul 10, 2015 at 1:48 PM, Sean Silva <chisophugis@gmail.com> wrote:
>
>
> On Wed, Jul 8, 2015 at 2:05 PM, Richard Smith <richard@metafoo.co.uk>
> wrote:
>
> > rsmith added inline comments.
> >
> > ================
> > Comment at: lib/Lex/PPDirectives.cpp:1665
> > @@ +1664,3 @@
> > + // unavailable, diagnose the situation and bail out.
> > + if (SuggestedModule && !SuggestedModule.getModule()->isAvailable()) {
> > + clang::Module::Requirement Requirement;
> > ----------------
> > I think this whole block should be moved down to line 1761 or so:
> >
> > 1. The code currently ensures that it always calls InclusionDirective on
> > the callback object for every `#include`, even if that include fails.
> >
>
> I don't think this is true. There is at least one return above it in the
> block `// We hit an error processing the import. Bail out.`.
>
That looks like a bug.
2. We don't yet know that the file is actually part of `SuggestedModule`;
> > it could be in the directory of an umbrella header whose module is
> > unavailable, but it might not be part of that module.
> >
>
> Sounds like a bug in the thing suggesting SuggestedModule? Or am I not
> understanding what the contract is for SuggestedModule? If I'm
> understanding what you're saying, it sounds like this is wholly not the
> correct place to check this.
>
This is the nature of umbrella headers: we don't know which headers they
cover until we build the corresponding module. SuggestedModule is just a
suggestion until we've tried to load it and checked whether the header is
actually covered by the module. (Yes, this is horrible, and I wish we
didn't have these.)
3. If we somehow got a suggested module but no file, we should not produce
> > additional spurious diagnostics beyond the "file not found" diagnostic we
> > already produced.
> >
>
> Is that actually possible? Sounds like a broken invariant.
>
Yes, I think you're right.
> -- Sean Silva
>
>
> >
> > ================
> > Comment at: test/Modules/Inputs/auto-import-unavailable/module.modulemap:9
> > @@ +8,3 @@
> > + requires nonexistent_feature
> > + header "nonrequired_missing_header/missing.h"
> > + }
> > ----------------
> > Please add some content to the empty files (even if just a comment) or
> > this test will misbehave on content-addressed file systems, where all the
> > empty files will be treated as the same file.
> >
> >
> > http://reviews.llvm.org/D10423
> > <https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10423&d=AwM \
> > FaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=tx12L \
> > oYOst8vPjqn56Wz7HAg_vR0GGQvI1wzFCoYuFM&s=JOYPVLm3SnA50c5_ZEeupNTAz-QVk71-e8eOivltI50&e=>
> >
> >
> >
> >
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 10, 2015 \
at 1:48 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 2:05 PM, Richard Smith \
<span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" \
target="_blank">richard@metafoo.co.uk</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">rsmith \
added inline comments.<br> <br>
================<br>
Comment at: lib/Lex/PPDirectives.cpp:1665<br>
@@ +1664,3 @@<br>
+ // unavailable, diagnose the situation and bail out.<br>
+ if (SuggestedModule && !SuggestedModule.getModule()->isAvailable()) \
{<br> + clang::Module::Requirement Requirement;<br>
----------------<br>
I think this whole block should be moved down to line 1761 or so:<br>
<br>
1. The code currently ensures that it always calls InclusionDirective on the \
callback object for every `#include`, even if that include \
fails.<br></blockquote><div><br></div></span><div>I don't think this is true. \
There is at least one return above it in the block `// We hit an error processing the \
import. Bail out.`.</div></div></div></div></blockquote><div><br></div><div>That \
looks like a bug.</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"><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">
2. We don't yet know that the file is actually part of `SuggestedModule`; it \
could be in the directory of an umbrella header whose module is unavailable, but it \
might not be part of that module.<br></blockquote><div><br></div></span><div>Sounds \
like a bug in the thing suggesting SuggestedModule? Or am I not understanding what \
the contract is for SuggestedModule? If I'm understanding what you're saying, \
it sounds like this is wholly not the correct place to check \
this.</div></div></div></div></blockquote><div><br></div><div>This is the nature of \
umbrella headers: we don't know which headers they cover until we build the \
corresponding module. SuggestedModule is just a suggestion until we've tried to \
load it and checked whether the header is actually covered by the module. (Yes, this \
is horrible, and I wish we didn't have these.)</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"><span class=""><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">
3. If we somehow got a suggested module but no file, we should not produce \
additional spurious diagnostics beyond the "file not found" diagnostic we \
already produced.<br></blockquote><div><br></div></span><div>Is that actually \
possible? Sounds like a broken \
invariant.</div></div></div></div></blockquote><div><br></div><div>Yes, I think \
you're right.</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">
<br>
================<br>
Comment at: test/Modules/Inputs/auto-import-unavailable/module.modulemap:9<br>
@@ +8,3 @@<br>
+ requires nonexistent_feature<br>
+ header "nonrequired_missing_header/missing.h"<br>
+ }<br>
----------------<br>
Please add some content to the empty files (even if just a comment) or this test will \
misbehave on content-addressed file systems, where all the empty files will be \
treated as the same file.<br> <br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10423&a \
mp;d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0 \
cCxk&m=tx12LoYOst8vPjqn56Wz7HAg_vR0GGQvI1wzFCoYuFM&s=JOYPVLm3SnA50c5_ZEeupNTAz-QVk71-e8eOivltI50&e=" \
rel="noreferrer" target="_blank">http://reviews.llvm.org/D10423</a><br> <br>
<br>
<br>
</blockquote></span></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
<br></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