r250577 - [modules] Allow the error when explicitly loading an incompatible module file

Sean Silva via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 19 20:52:38 PDT 2015 

On Mon, Oct 19, 2015 at 2:10 AM, Manuel Klimek <klimek at google.com> wrote:

> On Sat, Oct 17, 2015 at 3:41 AM Richard Smith via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> On Fri, Oct 16, 2015 at 6:30 PM, Sean Silva <chisophugis at gmail.com>
>> wrote:
>>
>>> On Fri, Oct 16, 2015 at 6:26 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>>> On Fri, Oct 16, 2015 at 6:25 PM, Sean Silva <chisophugis at gmail.com>
>>>> wrote:
>>>>
>>>>> On Fri, Oct 16, 2015 at 6:12 PM, Richard Smith <richard at metafoo.co.uk>
>>>>> wrote:
>>>>>
>>>>>> On Fri, Oct 16, 2015 at 4:43 PM, Sean Silva <chisophugis at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> On Fri, Oct 16, 2015 at 4:20 PM, Richard Smith via cfe-commits <
>>>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>>>
>>>>>>>> Author: rsmith
>>>>>>>> Date: Fri Oct 16 18:20:19 2015
>>>>>>>> New Revision: 250577
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=250577&view=rev
>>>>>>>> Log:
>>>>>>>> [modules] Allow the error when explicitly loading an incompatible
>>>>>>>> module file
>>>>>>>> via -fmodule-file= to be turned off; in that case, just include the
>>>>>>>> relevant
>>>>>>>> files textually. This allows module files to be unconditionally
>>>>>>>> passed to all
>>>>>>>> compile actions via CXXFLAGS, and to be ignored for rules that
>>>>>>>> specify custom
>>>>>>>> incompatible flags.
>>>>>>>>
>>>>>>>
>>>>>>> What direction are you trying to go with this? Are you thinking
>>>>>>> something like having CMake build a bunch of modules up front?
>>>>>>>
>>>>>>
>>>>>> That's certainly one thing you can do with this. Another is that you
>>>>>> can make cmake automatically and explicitly build a module for each
>>>>>> library, and then provide that for all the dependencies of that library,
>>>>>>
>>>>>
>>>>> How does CMake know which headers are part of which library?
>>>>> Strategically named top-level modules in the module map?
>>>>>
>>>>
>>>> The idea would be for CMake to generate the module map itself based on
>>>> the build rules.
>>>>
>>>
>>> How would it know which headers to include? Do
>>> our ADDITIONAL_HEADER_DIRS things in our CMakeLists.txt have enough
>>> information for this?
>>>
>>
>> Some additional information may need to be added to the CMakeLists to
>> enable this. Some build systems already model the headers for a library,
>> and so already have the requisite information.
>>
>
> CMake supports specifying headers for libraries (mainly used for MS VS).
> If we need this for modules, we'll probably need to update our build rules
> (which will probably make sense anyway, for a better experience for VS
> users ;)
>

Nice.

Brad, do you have any idea how hard it would be to get cmake to generate
clang module map files and add explicit module build steps? Basically, the
requirements (off the top of my head) are:
- for each library, generate a module map which is essentially just a list
of the headers in that library (it's not just a flat list, but that's the
gist of it).
- for each module map, add a build step that invokes clang on it to say
"build the module corresponding to this module map" (it's basically
`clang++ path/to/foo.modulemap -o foo.pcm` with a little bit of fluff
around it). There is also a dependency from foo.pcm on each of the
libraries that library "foo" depends on.
- for each library $Dep that library $Lib depends on, add $Dep's .pcm file
as a dependency of the .o build steps for $Lib. $Dep's .pcm file also needs
to be passed on the command line of the .o build steps for $Lib.

It seems like similar requirements are going to be common in the
standardized modules feature (except for the module map I think? Richard?).
Basically, in order to avoid redundantly parsing textual headers, you need
to run a build step on headers that turns them into some form that can be
processed more efficiently than just parsing it. E.g. the build step on
slide 36 of this cppcon presentation about the Microsoft experimental
modules implementation https://www.youtube.com/watch?v=RwdQA0pGWa4 (slides:
https://goo.gl/t4Eg89 ).

Let me know if there is anything I can do to help (up to and including
patches, but I'll need pointers and possibly some hand-holding as I'm
unfamiliar with the CMake language and CMake codebase).

There's also some issues of detecting if the host clang is new enough that
we want to use its modules feature and also the issue of detecting
modularized system headers if available, but we can hammer those things out
as we run into them.

Manuel, I heard through the grape vine that you were the one that
implemented the explicit modules stuff for bazel? Did I miss anything in my
list above?

Richard, are there any blockers to exposing a driver flag for explicit
modules?

-- Sean Silva


>
>
>>
>>
>>> -- Sean Silva
>>>
>>>
>>>>
>>>>
>>>>> -- Sean Silva
>>>>>
>>>>>
>>>>>> with an (error-by-default) warning in the case where the downstream
>>>>>> library specifies incompatible compilation flags. You can use this warning
>>>>>> flag to turn off the error so you can make progress before you get around
>>>>>> to fixing all the incompatible flags.
>>>>>>
>>>>>>
>>>>>>> If that's the case, it would be nice to explain what caused the
>>>>>>> mismatch, so that the user can look into rectifying it. Otherwise this
>>>>>>> warning is not directly actionable. The existing diagnostics seemed
>>>>>>> alright. Demoting them to "error: {{.*}} configuration mismatch" seems like
>>>>>>> a regression.
>>>>>>>
>>>>>>
>>>>>> I agree, it is a regression, and fixing it is high on my list of
>>>>>> priorities (sorry for not mentioning that in the commit message).
>>>>>>
>>>>>> -- Sean Silva
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>     cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
>>>>>>>>     cfe/trunk/lib/Frontend/CompilerInstance.cpp
>>>>>>>>     cfe/trunk/test/Modules/merge-target-features.cpp
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td?rev=250577&r1=250576&r2=250577&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
>>>>>>>> (original)
>>>>>>>> +++ cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td Fri
>>>>>>>> Oct 16 18:20:19 2015
>>>>>>>> @@ -172,6 +172,9 @@ def warn_incompatible_analyzer_plugin_ap
>>>>>>>>  def note_incompatible_analyzer_plugin_api : Note<
>>>>>>>>      "current API version is '%0', but plugin was compiled with
>>>>>>>> version '%1'">;
>>>>>>>>
>>>>>>>> +def warn_module_config_mismatch : Warning<
>>>>>>>> +  "module file %0 cannot be loaded due to a configuration mismatch
>>>>>>>> with the current "
>>>>>>>> +  "compilation">,
>>>>>>>> InGroup<DiagGroup<"module-file-config-mismatch">>, DefaultError;
>>>>>>>>  def err_module_map_not_found : Error<"module map file '%0' not
>>>>>>>> found">,
>>>>>>>>    DefaultFatal;
>>>>>>>>  def err_missing_module_name : Error<
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=250577&r1=250576&r2=250577&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
>>>>>>>> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Fri Oct 16 18:20:19
>>>>>>>> 2015
>>>>>>>> @@ -1335,15 +1335,24 @@ bool CompilerInstance::loadModuleFile(St
>>>>>>>>
>>>>>>>> std::move(Listener));
>>>>>>>>
>>>>>>>>    // Try to load the module file.
>>>>>>>> -  if (ModuleManager->ReadAST(FileName,
>>>>>>>> serialization::MK_ExplicitModule,
>>>>>>>> -                             SourceLocation(), ASTReader::ARR_None)
>>>>>>>> -          != ASTReader::Success)
>>>>>>>> -    return false;
>>>>>>>> -
>>>>>>>> +  switch (ModuleManager->ReadAST(FileName,
>>>>>>>> serialization::MK_ExplicitModule,
>>>>>>>> +                                 SourceLocation(),
>>>>>>>> +
>>>>>>>>  ASTReader::ARR_ConfigurationMismatch)) {
>>>>>>>> +  case ASTReader::Success:
>>>>>>>>    // We successfully loaded the module file; remember the set of
>>>>>>>> provided
>>>>>>>>    // modules so that we don't try to load implicit modules for
>>>>>>>> them.
>>>>>>>>    ListenerRef.registerAll();
>>>>>>>>    return true;
>>>>>>>> +
>>>>>>>> +  case ASTReader::ConfigurationMismatch:
>>>>>>>> +    // Ignore unusable module files.
>>>>>>>> +    getDiagnostics().Report(SourceLocation(),
>>>>>>>> diag::warn_module_config_mismatch)
>>>>>>>> +        << FileName;
>>>>>>>> +    return true;
>>>>>>>> +
>>>>>>>> +  default:
>>>>>>>> +    return false;
>>>>>>>> +  }
>>>>>>>>  }
>>>>>>>>
>>>>>>>>  ModuleLoadResult
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/test/Modules/merge-target-features.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/merge-target-features.cpp?rev=250577&r1=250576&r2=250577&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/test/Modules/merge-target-features.cpp (original)
>>>>>>>> +++ cfe/trunk/test/Modules/merge-target-features.cpp Fri Oct 16
>>>>>>>> 18:20:19 2015
>>>>>>>> @@ -20,7 +20,7 @@
>>>>>>>>  // RUN:   -target-cpu i386 \
>>>>>>>>  // RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
>>>>>>>>  // RUN:   | FileCheck --check-prefix=SUBSET %s
>>>>>>>> -// SUBSET: AST file was compiled with the target feature'+sse2'
>>>>>>>> but the current translation unit is not
>>>>>>>> +// SUBSET: error: {{.*}} configuration mismatch
>>>>>>>>  //
>>>>>>>>  // RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
>>>>>>>>  // RUN:   -iquote Inputs/merge-target-features \
>>>>>>>> @@ -56,8 +56,7 @@
>>>>>>>>  // RUN:   -target-cpu i386 -target-feature +cx16 \
>>>>>>>>  // RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
>>>>>>>>  // RUN:   | FileCheck --check-prefix=MISMATCH %s
>>>>>>>> -// MISMATCH: AST file was compiled with the target feature'+sse2'
>>>>>>>> but the current translation unit is not
>>>>>>>> -// MISMATCH: current translation unit was compiled with the target
>>>>>>>> feature'+cx16' but the AST file was not
>>>>>>>> +// MISMATCH: error: {{.*}} configuration mismatch
>>>>>>>>
>>>>>>>>  #include "foo.h"
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> cfe-commits mailing list
>>>>>>>> cfe-commits at lists.llvm.org
>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151019/41f6a4eb/attachment-0001.html>

More information about the cfe-commits mailing list