VerifyDiagnosticConsumer and ModuleMapParser

Thu Nov 6 09:45:03 PST 2014

> On Nov 3, 2014, at 2:38 PM, Vassil Vassilev <vvasilev at cern.ch> wrote:
> 
> On 03/11/14 16:49, Ben Langmuir wrote:
>>> On Nov 3, 2014, at 2:10 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:
>>> 
>>> Hi guys,
>>>  I am working on http://llvm.org/bugs/show_bug.cgi?id=20507 Now the diagnostic gets issued for:
>>>    Clang :: Modules/declare-use1.cpp
>>>    Clang :: Modules/declare-use2.cpp
>>>    Clang :: Modules/declare-use3.cpp
>>>    Clang :: Modules/strict-decluse.cpp
>>> 
>>> It says smth like: Modules/Inputs/declare-use/module.map:30:10: note: Header file 'unavailable.h' not present in module 'XF'
>>> 
>>> I'd like to add an expected diag to the modulemap file. Eg:
>>> module XF {
>>>  ...
>>>  header "unavailable.h" // expected-note {{...}}
>>>  ...
>>> }
>>> 
>>> Is that the right way to go?
>>> 
>>> If yes, VerifyDiagnosticConsumer requires some callbacks (such as HandleComment) which come from the Preprocessor. In the ModuleMapParser we use raw lexing (without PP at all) and I was wondering what would be the right way to go, in order to make the -verify flag work inside the module maps. One solution that I see is to pass the comment handlers from the PP to the ModuleMapParser, however IMO this would break the encapsulation. Do you have better ideas?
>> Rather than complicate the module map parsing, we could always put the // expected line into the importing source file (using @file:line).  I thought this would Just Work (TM), but when I tried some silly examples the verification didn’t work.  I don’t know if that’s related to it being a module map file or something to do with the fatal error.  I’m not sure what’s going wrong there, but that’s the direction I would explore.
> Thanks for the pointers. I have prepared a starter already. Wording of the diagnostic probably needs rewording :) Do you mind having a look?  Thanks!

Sorry for the delay!  Some comments:

> Index: include/clang/Basic/DiagnosticCommonKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticCommonKinds.td	(revision 220824)
> +++ include/clang/Basic/DiagnosticCommonKinds.td	(working copy)
> @@ -141,6 +141,7 @@
>    
>  // Modules
>  def err_module_file_conflict : Error<"module '%0' found in both '%1' and '%2'”>;
> +def note_module_header_missing : Note<"Header file '%0' not present in module '%1'”>;

We don’t capitalize the start of diagnostic messages.  I suggest something like:
“header ‘%0’ required by module ‘%1’ not found”
or even
“header ‘%0’ not found"

>  
>  // TransformActions
>  // TODO: Use a custom category name to distinguish rewriter errors.
> Index: lib/Lex/ModuleMap.cpp
> ===================================================================
> --- lib/Lex/ModuleMap.cpp	(revision 220824)
> +++ lib/Lex/ModuleMap.cpp	(working copy)
> @@ -311,6 +311,15 @@
>    }
>  }
>  
> +void ModuleMap::diagnoseModuleMissingRequirements(Module *M) {
> +  assert(M && "Must not be null");
> +  for(auto& Header: M->MissingHeaders) {
> +    Diags.Report(Header.FileNameLoc, diag::note_module_header_missing)
> +      << Header.FileName
> +      << M->getFullModuleName();
> +  }
> +}

This is using notes, but they need to be attached to an error (maybe you just haven’t added this yet?).  I would have something like:

#include “foo.h” // error: could not import module ‘Foo’
// @ Foo’s module map: note: header ‘bar.h’ required by module ‘Foo’ not found
// @ Foo’s module map: note: header ‘baz.h’ required by module ‘Foo’ not found

As you have already discovered in your test cases below, this is going to overlap with the “-fmodule-strict-decluse” and “-Wnon-modular-include” diagnostics when we fall back to the textual include.  We need to suppress those diagnostics when we diagnose a failed import like this, or we need to make the failed import a fatal error.  I would probably go with the former option, what do you think?

> +
>  ModuleMap::KnownHeader
>  ModuleMap::findModuleForHeader(const FileEntry *File,
>                                 Module *RequestingModule,
> @@ -331,8 +340,11 @@
>                                                  E = Known->second.end();
>           I != E; ++I) {
>        // Cannot use a module if it is unavailable.
> -      if (!I->getModule()->isAvailable())
> +      if (!I->getModule()->isAvailable()) {
> +        //TODO: Propagate the Loc of the requesting header.
> +        diagnoseModuleMissingRequirements(I->getModule()/*, IncludeLoc*/);
>          continue;
> +      }
>  
>        // If 'File' is part of 'RequestingModule', 'RequestingModule' is the
>        // module we are looking for.
> Index: test/Modules/declare-use2.cpp
> ===================================================================
> --- test/Modules/declare-use2.cpp	(revision 220824)
> +++ test/Modules/declare-use2.cpp	(working copy)
> @@ -3,5 +3,5 @@
>  
>  #include "h.h"
>  #include "e.h"
> -#include "f.h" // expected-error {{module XH does not depend on a module exporting 'f.h'}}
> +#include "f.h" // expected-error {{module XH does not depend on a module exporting 'f.h'}} // expected-note at module.map:30 {{Header file 'unavailable.h' not present in module 'XF'}}
>  const int h2 = h1+e+f;
> Index: test/Modules/Inputs/declare-use/f.h
> ===================================================================
> --- test/Modules/Inputs/declare-use/f.h	(revision 220824)
> +++ test/Modules/Inputs/declare-use/f.h	(working copy)
> @@ -1,6 +1,6 @@
>  #ifndef F_H
>  #define F_H
> -#include "a.h"
> -#include "b.h"
> +#include "a.h" // expected-note at module.map:30 {{Header file 'unavailable.h' not present in module 'XF'}}
> +#include "b.h" // expected-note at module.map:30 {{Header file 'unavailable.h' not present in module 'XF'}}
>  const int f = a+b;
>  #endif
> Index: test/Modules/Inputs/declare-use/h.h
> ===================================================================
> --- test/Modules/Inputs/declare-use/h.h	(revision 220824)
> +++ test/Modules/Inputs/declare-use/h.h	(working copy)
> @@ -1,7 +1,7 @@
>  #ifndef H_H
>  #define H_H
> -#include "c.h"
> -#include "d.h" // expected-error {{does not depend on a module exporting}}
> -#include "h1.h"
> +#include "c.h" // expected-note at module.map:23 {{Header file 'unavailable.h' not present in module 'XE'}}
> +#include "d.h" // expected-error {{does not depend on a module exporting}} // expected-note at module.map:23 {{Header file 'unavailable.h' not present in module 'XE'}}
> +#include "h1.h" // expected-note at module.map:23 {{Header file 'unavailable.h' not present in module 'XE'}}
>  const int h1 = aux_h*c*7*d;
>  #endif
> Index: include/clang/Lex/ModuleMap.h
> ===================================================================
> --- include/clang/Lex/ModuleMap.h	(revision 220824)
> +++ include/clang/Lex/ModuleMap.h	(working copy)
> @@ -274,6 +274,10 @@
>    void diagnoseHeaderInclusion(Module *RequestingModule,
>                                 SourceLocation FilenameLoc, StringRef Filename,
>                                 const FileEntry *File);
> +  /// \brief Reports issues when a module declaration doesn't match the
> +  /// requirements and will be marked as not available.
> +  ///
> +  void diagnoseModuleMissingRequirements(Module *M);
>  
>    /// \brief Determine whether the given header is part of a module
>    /// marked 'unavailable'.
> 

> Vassil
>> 
>> Ben
>> 
>>> Many thanks,
>>> Vassil
> 
> 
> -- 
> --------------------------------------------
> Q: Why is this email five sentences or less?
> A: http://five.sentenc.es
> 
> <bug20507.diff>