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

Manuel Klimek via cfe-commits cfe-commits at lists.llvm.org
Sun Nov 1 10:20:17 PST 2015


On Fri, Oct 23, 2015 at 9:31 PM Sean Silva <chisophugis at gmail.com> wrote:

> 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?
>

We use -x c++ -Xclang=emit-module.
What exactly is not working?


> 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?
>

No


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

We're actively working on removing all compiler flags from bazel. Instead,
bazel uses a toolchain definition file with all flags. Unfortunately, the
current one does not include support for modules yet, as we're first
working on getting it set up internally.


>
> -- 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/20151101/5458b324/attachment-0001.html>


More information about the cfe-commits mailing list