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

[Sean Silva]

On Wed, Jul 1, 2015 at 3:32 PM, Ben Langmuir <blangmuir at apple.com> wrote:

> I tried this patch out and found a few issues I've commented on inline.
> It also exposes the two issues below:
>
> 1. This patch exposes a compatibility issue with our 'Darwin' module map
> file (the module map that covers most of /usr/include on Darwin platforms).
>
>   error: module 'Darwin.C.excluded' requires feature 'excluded'
>
> This "excluded" submodule was used as a hack for "assert.h" before the
> explicit support for "exclude" and "textual" headers were added to clang.
> After this patch, it won't be possible to include assert.h on Darwin,
> because the module it is in is missing a requirement.  I think it would be
> very bad to break compatibility here, so I propose we add a compatibility
> hack to module map parsing that treats a module with a the requirement
> "excluded" as if all its headers were declared with "exclude".  What do you
> think?  I'm happy to implement this.
>

Thanks for looking into this! Yeah, practically speaking we need to keep
things working for existing module maps. I'll make sure to sync up with you
regarding any pragmatic hacks before committing.


>
> 2. If a require decl is written *after* a header decl in a module, that
> header still gets treated as required.  We should fix that before this is
> committed.  E.g.
>
>
>
>   module Top {
>     module A { header "foo.h" requires nope }
>     module B { requires nope header "bar.h" }
>   }
>
> We'll complain about missing foo.h, but not about bar.h.


>
> ================
> Comment at: lib/Lex/PPDirectives.cpp:1671
> @@ +1670,3 @@
> +      Diag(MissingHeader.FileNameLoc, diag::err_module_unavailable)
> +          << M->getFullModuleName() << Requirement.second <<
> Requirement.first;
> +    }
> ----------------
> 1. This uses MissingHeader.FileNameLoc, which we just proved is invalid :-)
> 2. We should also show diag::note_implicit_top_level_module_import_here in
> this diagnostic.
>
> ================
> Comment at: test/Modules/missing-header.cpp:13
> @@ +12,2 @@
> +// We should not attempt to build the module.
> +// CHECK-NOT: could not build module 'missing_headers'
> ----------------
> We should have a test for including a header from a module with a missing
> requirement.  And one where a missing header from a different submodule is
> okay because the submodule is missing a requirement.
>

Oops. Yeah, I came at this from the direction of missing headers, and sort
of wasn't paying much attention to the 'requires' case. But it seems like
that one is finding some bugs too :)

I'll try to get an updated patch out tomorrow.

-- Sean Silva


>
> http://reviews.llvm.org/D10423
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150701/e427cc77/attachment.html>