On Sun, Nov 1, 2015 at 10:20 AM, Manuel Klimek <kli...@google.com> wrote:
> On Fri, Oct 23, 2015 at 9:31 PM Sean Silva <chisophu...@gmail.com> wrote: > >> On Tue, Oct 20, 2015 at 1:52 AM, Manuel Klimek <kli...@google.com> wrote: >> >>> On Tue, Oct 20, 2015 at 10:41 AM Sean Silva <chisophu...@gmail.com> >>> wrote: >>> >>>> On Tue, Oct 20, 2015 at 1:38 AM, Manuel Klimek <kli...@google.com> >>>> wrote: >>>> >>>>> On Tue, Oct 20, 2015 at 5:52 AM Sean Silva <chisophu...@gmail.com> >>>>> wrote: >>>>> >>>>>> On Mon, Oct 19, 2015 at 2:10 AM, Manuel Klimek <kli...@google.com> >>>>>> wrote: >>>>>> >>>>>>> On Sat, Oct 17, 2015 at 3:41 AM Richard Smith via cfe-commits < >>>>>>> cfe-commits@lists.llvm.org> wrote: >>>>>>> >>>>>>>> On Fri, Oct 16, 2015 at 6:30 PM, Sean Silva <chisophu...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> On Fri, Oct 16, 2015 at 6:26 PM, Richard Smith < >>>>>>>>> rich...@metafoo.co.uk> wrote: >>>>>>>>> >>>>>>>>>> On Fri, Oct 16, 2015 at 6:25 PM, Sean Silva < >>>>>>>>>> chisophu...@gmail.com> wrote: >>>>>>>>>> >>>>>>>>>>> On Fri, Oct 16, 2015 at 6:12 PM, Richard Smith < >>>>>>>>>>> rich...@metafoo.co.uk> wrote: >>>>>>>>>>> >>>>>>>>>>>> On Fri, Oct 16, 2015 at 4:43 PM, Sean Silva < >>>>>>>>>>>> chisophu...@gmail.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Oct 16, 2015 at 4:20 PM, Richard Smith via cfe-commits >>>>>>>>>>>>> <cfe-commits@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.) 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@lists.llvm.org >>>>>>>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>> cfe-commits mailing list >>>>>>>> cfe-commits@lists.llvm.org >>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>>>> >>>>>>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
