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

Sean Silva via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 23 21:31:32 PDT 2015 

On Tue, Oct 20, 2015 at 1:52 AM, Manuel Klimek <klimek at google.com> wrote:

> On Tue, Oct 20, 2015 at 10:41 AM Sean Silva <chisophugis at gmail.com> wrote:
>
>> On Tue, Oct 20, 2015 at 1:38 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> On Tue, Oct 20, 2015 at 5:52 AM Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>> 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?
>>>>
>>>
>>> I think that's about right. We also embed the module maps into the
>>> modules, but most of these things only matter for distributed builds at
>>> scale.
>>>
>>> Also, I have some experience hacking on cmake, and from my experience I
>>> think this shouldn't be too hard to get working (mainly work ;)
>>>
>>
>> Thanks, my CMake-fu is weak. Any help from doing it yourself to pointing
>> me in the right direction is much appreciated.
>>
>>
>>>
>>>
>>>> Richard, are there any blockers to exposing a driver flag for explicit
>>>> modules?
>>>>
>>>
>>> Which flag are you missing?
>>>
>>
>> IIRC -emit-module cannot be accessed from the driver?
>>
>
> Ah, you're right (well, all flags can be accessed via the driver by saying
> -Xclang -flag, right?)
>

Actually, despite a decent amount of experimenting and looking at the
source, I can't seem to find a way to do this without invoking cc1
directly. (basically, it seems like there is no way to avoid -emit-obj or
another not-"-emit-module" action while still getting all the right
driver-y stuff passed down). Is there some magic invocation?

I looked inside of Bazel (current master 76fa4a4) and all -Xclang flags
seem to be PGO-related. In 2fd9960f you seem to have removed mention of
them with commit message "Get rid of legacy default features that are not
needed any more.". Do you guys have private driver patches for building
with modules? Could you push those upstream? Also grepping for -fmodules
only finds objc related stuff, so I'm wondering how Bazel is even turning
on the modules feature.

-- Sean Silva



>
>
>>
>> -- Sean Silva
>>
>>
>>>
>>>
>>>>
>>>> -- 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/20151023/153d12b7/attachment-0001.html>

More information about the cfe-commits mailing list