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

[Ben Langmuir]

> On Jul 2, 2015, at 5:02 PM, Sean Silva <chisophugis at gmail.com> wrote:
> 
> 
> 
> On Wed, Jul 1, 2015 at 3:32 PM, Ben Langmuir <blangmuir at apple.com <mailto: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?

You can see this by modifying your test case ``nonrequired_missing_header``:

  module nonrequired_missing_header {
    module unsatisfied_requires {
      requires nonexistent_feature
      header "nonrequired_missing_header/missing.h”
    }
+   module unsatisfied_requires_after {
+     header "nonrequired_missing_header/missing2.h”
+     requires nonexistent_feature
+   }
    module fine_to_import {
      header "nonrequired_missing_header/not_missing.h"
    }
  }


error: header 'nonrequired_missing_header/missing2.h' not found

Ben

> 
> -- 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 <http://reviews.llvm.org/D10423>
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150708/8d47acd2/attachment.html>