[PATCH] D10423: [modules] PR20507: Avoid silent textual inclusion.

[Richard Smith]

On Fri, Jul 10, 2015 at 1:48 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Wed, Jul 8, 2015 at 2:05 PM, Richard Smith <richard at 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=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=tx12LoYOst8vPjqn56Wz7HAg_vR0GGQvI1wzFCoYuFM&s=JOYPVLm3SnA50c5_ZEeupNTAz-QVk71-e8eOivltI50&e=>
>>
>>
>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150713/1632a74e/attachment.html>