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

[Sean Silva]

On Thu, Jul 9, 2015 at 4:06 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, Jul 9, 2015 at 3:47 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Wed, Jul 8, 2015 at 12:58 PM, Ben Langmuir <blangmuir at 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)
>
> ?
>

No (I assume you meant the check in the other direction). Actually even my
patch fails in some cases. There are other ways that things go wrong, at
least the way we mark the top-level module as unavailable when we see a
missing header. E.g.

module nonrequired_missing_header {

  module unsatisfied_requires {

    header "nonrequired_missing_header/missing.h"

+    module foo {

+      header "nonrequired_missing_header/foomissing.h"

+    }

    requires nonexistent_feature

  }

  module fine_to_import {

    header "nonrequired_missing_header/not_missing.h"

  }

}


> 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?
>

I don't think so. At worst the current patch will trade a silent textual
inclusion for a spurious missing header error. Since our previous behavior
was not correct, it cannot be a regression. Anecdotally, a silent textual
inclusion can be insanely hard to debug and so a spurious error with an
easy-to-understand workaround is an improvement, if anything.

-- Sean Silva


>
>> -- 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
>>>
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150710/6119bdce/attachment.html>