r250577 - [modules] Allow the error when explicitly loading an incompatible module file
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 5 13:12:27 PST 2015
On Wed, Nov 4, 2015 at 7:14 PM, Sean Silva via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> On Wed, Nov 4, 2015 at 1:07 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On Sun, Nov 1, 2015 at 10:20 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> 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.
>>>
>>
>> We should add a way of doing this without passing cc1 flags through the
>> driver. Maybe:
>>
>> clang -x c++-module my_module_name
>>
>> (...though it's a bit weird to pass something other than a file name as
>> an input to the driver.)
>>
>
> Wouldn't this have to be: clang -x c++-module my_module.modulemap
> -fmodule-name=my_module_name ?
>
We can pick whatever command-line syntax we like =)
It makes more sense to me to pass the module map file via
-fmodule-map-file= rather than treating one of the input module maps as a
special case.
Also, we would need an equivalent of `-fmodule-map-file-home-is-cwd` which
> is necessary for sane explicit module builds. (for the record, I find that
> option name very confusing because despite all my efforts to the contrary,
> I still read it as having a subphrase "home is cwd" which makes it sound
> like it has something to do with `~` expansion or something; maybe we could
> generalize it to `-fmodule-map-header-path-root=.` or something?).
>
We may also want to be able to specify a different value for each
explicitly-specified module map file. I was thinking about something like:
-fmodule-map-file=.:foo/bar.modulemap
where the piece before the : is the root directory for modules defined in
that module map file. But separate flags could also work if we preserve
their order and use the most recent one prior to the -fmodule-map-file=
argument or similar.
> -- Sean Silva
>
>
>>
>> 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
>>>>>>>>>>
>>>>>>>>>
>>
>
> _______________________________________________
> 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/20151105/ee01e02b/attachment-0001.html>
More information about the cfe-commits
mailing list