r250577 - [modules] Allow the error when explicitly loading an incompatible module file
Sean Silva via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 4 19:14:49 PST 2015
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 ?
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?).
-- 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
>>>>>>>>>
>>>>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151104/81013f7d/attachment-0001.html>
More information about the cfe-commits
mailing list