r250577 - [modules] Allow the error when explicitly loading an incompatible module file
Manuel Klimek via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 20 01:52:53 PDT 2015
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?)
>
> -- 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/20151020/55cccd18/attachment-0001.html>
More information about the cfe-commits
mailing list