[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.
>
> 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.
>

I can't reproduce this locally (with or without my patch). What concrete
command line are you using, and what are you seeing?

-- Sean Silva


>
>
> ================
> 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.
>
> 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/20150702/9d1ae695/attachment.html>