[cfe-dev] [Modules] Silent textual inclusion when files not found.

[Sean Silva]

On Thu, Jun 4, 2015 at 2:18 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Thu, Jun 4, 2015 at 1:49 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> On Thu, Jun 4, 2015 at 1:27 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> On Wed, Jun 3, 2015 at 6:24 PM, Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>> On Wed, Jun 3, 2015 at 8:40 AM, Ben Langmuir <blangmuir at apple.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Jun 2, 2015, at 7:57 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Currently we do not seem to issue any form of diagnostic when there
>>>>> are missing headers named in a module map. See the attached test case. Even
>>>>> worse, we will just treat all headers in the module as textual. Hilarity
>>>>> ensues.
>>>>>
>>>>>
>>>>> Yep, this is llvm.org/PR20507
>>>>>
>>>>>
>>>>> Some prior art in this area:
>>>>> Daniel - r197485
>>>>> Ben - r206664
>>>>>
>>>>> Based on these commits, it ostensibly seems that clang does *some*
>>>>> sort of checking for missing files in a module map, but I can't seem to
>>>>> coax clang into doing this in C++ language mode.
>>>>>
>>>>> Looking at the source code, it seems like we end up conflating
>>>>> "unavailable" due to a failed `requires` with "a header is missing". It
>>>>> sounds like we essentially need two notions "unsatisfied `requires`" and
>>>>> "necessary header is missing" (a header guarded by an unsatisfied
>>>>> `requires` does not count as "necessary"). Haven't dug in deep yet, but my
>>>>> hypothesis is that somewhere along the way we silently treat a module with
>>>>> missing headers as though it had an unsatisfied `requires`, leading to us
>>>>> silently neglecting that it was ever in a module at all.
>>>>>
>>>>>
>>>>> Yes, they both come out as the module being “unvavailable”, which is
>>>>> why we don’t import it.  I think the right answer is that attempting to
>>>>> import any unavailable module (even via an auto-import) should be an
>>>>> error.  That is:
>>>>>
>>>>> module MissingHeader {
>>>>>     header “exists.h"
>>>>>     header “doesnt_exist.h”
>>>>> }
>>>>>
>>>>> #include <exists.h> // error, this would import MissingHeader, which
>>>>> is unavailable because we're missing header “doesnt_exist.h"
>>>>>
>>>>> module Top {
>>>>>     header “Top.h”
>>>>>     module A {
>>>>>         requires non_existent
>>>>>        header “A.h”
>>>>>     }
>>>>>     module B {
>>>>>         header “B.h”
>>>>>     }
>>>>> }
>>>>>
>>>>> #include <Top.h> // OK
>>>>> #include <A.h> // error, this would import Top.A, which is unavailable
>>>>> because of the missing requirment
>>>>> #include <B.h> // OK
>>>>>
>>>>> But handling the first case is more important I think.
>>>>>
>>>>>
>>>>> I'm glad to put some effort into fixing this; I spent a good part of
>>>>> today with a bizarre error that I traced back to this and I don't want my
>>>>> users to have to deal with the same. Any pointers would be appreciated.
>>>>>
>>>>>
>>>>> That would be great! I was coincidentally hoping to fix this myself in
>>>>> the next few weeks, but if you beat me to it even better.
>>>>>
>>>>>
>>>> I'm still acclimating myself to the modules code, do you have any
>>>> concrete thoughts on how to do this? I was just planning on looking through
>>>> for everywhere we use "unavailable" happens and figure out a pattern, but
>>>> if you already have an idea, it might save me some time.
>>>>
>>>
>>> Something like:
>>>  * make ModuleMap::findModuleForHeader able to produce an unavailable
>>> module, but treat it as being worse than any available module
>>>  * make Preprocessor::HandleIncludeDirective issue an error if its call
>>> to LookupFile produces a SuggestedModule that is unavailable
>>>
>>> This should probably happen even if modules is disabled (that is, when
>>> we are parsing module maps but not actually building / using precompiled
>>> module files).
>>>
>>
>> Thanks for the pointers. I'll take a look at doing this.
>>
>>
>>>
>>> Also, any preferred bikeshed for the names for "unvailable because of
>>>> unsatisfied requires" and "necessary header is missing" concepts in the
>>>> source?
>>>>
>>>
>>> If the response to both is the same (as Ben suggested, and I agree) then
>>> I'm not sure that we need to distinguish them anywhere other than the place
>>> where we produce the diagnostic.
>>>
>>
>> But aren't they different when it comes time for clang to build the
>> module? We want to build a top-level module even if it has a submodule with
>> an unsatisfied "requires" (e.g. _Builtin_intrinsics has submodules with
>> contradictory `requires`), taking appropriate action to exclude any headers
>> with unsatisfied `requires`. On the other hand (modulo submodules already
>> removed from consideration due to unsatisfied `requires`), a missing header
>> should be treated as a hard error.
>>
>
> I think you're right, but I think that will be the emergent behavior from
> treating the two cases the same: if a module's header is missing, the
> corresponding *top-level* module (and all its submodules) gets marked
> unavailable, so we should never try to implicitly build it.
>

Thanks for the explanation, that makes perfect sense.

-- Sean Silva


>
> There might be a file system race here, where the main build process
> thinks the module is available, but the module build finds a header is
> missing and builds an empty module because all the submodules are
> unavailable. That's probably worth fixing, but again the fix doesn't really
> depend on the different kinds of availability: if the top-level module is
> unavailable, we should produce an error if we try to build a module file
> for it. (This is probably easy to test by building an unavailable module
> with -cc1 -emit-module.)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150604/e6149b53/attachment.html>