r206027 - Add -fmodules-strict-decluse to check that all headers are in modules
Manuel Klimek
klimek at google.com
Tue Apr 22 02:40:39 PDT 2014
On Thu, Apr 17, 2014 at 7:02 PM, Richard Smith <richard at metafoo.co.uk>wrote:
> On Thu, Apr 17, 2014 at 3:28 AM, Daniel Jasper <djasper at google.com> wrote:
>
>> On Wed, Apr 16, 2014 at 10:42 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> On Wed, Apr 16, 2014 at 12:24 PM, Daniel Jasper <djasper at google.com>wrote:
>>>
>>>> On Wed, Apr 16, 2014 at 7:45 PM, Ben Langmuir <blangmuir at apple.com>wrote:
>>>>
>>>>> On Apr 16, 2014, at 10:28 AM, Richard Smith <richard at metafoo.co.uk>
>>>>> wrote:
>>>>>
>>>>> On Wed, Apr 16, 2014 at 9:38 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>>>>
>>>>>>
>>>>>> On Apr 14, 2014, at 5:47 PM, Richard Smith <richard at metafoo.co.uk>
>>>>>> wrote:
>>>>>>
>>>>>> On Mon, Apr 14, 2014 at 11:53 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>>>>>
>>>>>>> To be clear, my patch does not involve decluse, so not quite the
>>>>>>> same.
>>>>>>>
>>>>>>
>>>>>> There's no reason for this patch to be restricted to decluse (just as
>>>>>> there's no reason for yours to be restricted to framework modules in the
>>>>>> long term). The underlying intent is the same, and it doesn't make sense to
>>>>>> have two different implementations of it.
>>>>>>
>>>>>>
>>>>>> Are you suggesting folding these into one diagnostic and -W option?
>>>>>> If so, is there a reasonable way to restrict it to frameworks on the
>>>>>> command line, since that is an important restriction to us for now
>>>>>> (unfortunately).
>>>>>>
>>>>>> Also, unlike decluse this is intended to be transitive (although I’m
>>>>>> not sure that the below patch is *not* transitive… I just know that’s the
>>>>>> intent for decluse). Is that okay?
>>>>>>
>>>>>
>>>>> They're both intended to be transitive, AFAIK.
>>>>>
>>>>>
>>>>> Interesting, that’s not what the comments
>>>>> in test/Modules/Inputs/declare-use/module.map say. Transitive makes more
>>>>> sense to me anyway.
>>>>>
>>>>
>>>> decluse is currently explicitly not meant to be transitive. We only
>>>> want to check whether the module we are currently building explicitly
>>>> specifies its uses.
>>>>
>>>
>>> What does this mean if building one module causes us to build another
>>> module that violates the decl-ref rules? (That is, when you're actually
>>> building modules and not just using module maps for dependency checking?)
>>>
>>
>> Well, then we should issue a warning, when we are building that module
>> transitively. But if say, we are building module A and that requires module
>> B and then B has previously been built ignoring the warning. Then I think
>> we should not complain about that missing usage again.
>>
>
> That's what we *will* do, but I don't think it's what we *should* do: the
> warnings we produce for a build really shouldn't depend on what happens to
> be in the module cache when the build begins. (Ultimately, this is one of
> the weaknesses making the module build step implicit, and eventually I
> think we'll want to give people an easy way to make it explicit.)
>
1. I assumed when using a build system the modules "cache" would basically
always be prepopulated with the result of the compilation of the
dependencies (very much like Java builds do)
2. I strongly disagree: I think the warning of a module being compiled
should always be the same, independently whether its compile happens to be
triggered from a different module compilation; note that afaiu this is the
way it happens to work in other languages that have similar "auto-build"
triggering logic (I cross-checked how Java does it); if clang wants to go
down a different path, I'd expect to have a very compelling argument to do
so.
Cheers,
/Manuel
>
> Nonetheless, it seems that we can still use exactly the same model for
> your warning and Ben's: warn on any direct inclusions into the current
> module (and not in inclusions into things that are included into that
> module). If/when we build the dependency modules, we'll warn on their
> direct inclusions, and so on.
>
> Here’s a strawman proposal:
>>>>>
>>>>> -Wnon-modular-include -- warn on any #include of a header that's not
>>>>> listed in a module map
>>>>> [-Wnon-modular-include-in-module -- subgroup, only warn if the
>>>>> include is in a module]
>>>>> -Wnon-modular-include-in-framework-module -- subgroup, only warn
>>>>> if the include is in a framework module
>>>>>
>>>>> I don't think we have any demand for the middle option today, but I
>>>>> think it's what you guys want eventually?
>>>>>
>>>>>
>>>>> SGTM.
>>>>>
>>>>>
>>>>> Ben
>>>>>>
>>>>>>
>>>>>> Also (by way of review), I think this patch doesn’t handle umbrella
>>>>>>> directories correctly, since it only looks for known headers.
>>>>>>>
>>>>>>
>>>>>> Hmm, if we're getting that part wrong, we'll be getting it wrong for
>>>>>> the decluse checking in general. Nice catch =)
>>>>>>
>>>>>>
>>>>>>> On Apr 14, 2014, at 11:42 AM, Richard Smith <richard at metafoo.co.uk>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Why is this a -f option rather than a diagnostic? Why is it coupled
>>>>>>> to -fmodules-decluse? See also Ben Langmuir's patch that does the same
>>>>>>> thing for framework modules (also, oddly, with a language option).
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Apr 11, 2014 at 4:47 AM, Daniel Jasper <djasper at google.com>wrote:
>>>>>>>
>>>>>>>> Author: djasper
>>>>>>>> Date: Fri Apr 11 06:47:45 2014
>>>>>>>> New Revision: 206027
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=206027&view=rev
>>>>>>>> Log:
>>>>>>>> Add -fmodules-strict-decluse to check that all headers are in
>>>>>>>> modules
>>>>>>>>
>>>>>>>> Review: http://reviews.llvm.org/D3335
>>>>>>>>
>>>>>>>> Added:
>>>>>>>> cfe/trunk/test/Modules/strict-decluse.cpp
>>>>>>>> Modified:
>>>>>>>> cfe/trunk/include/clang/Basic/LangOptions.def
>>>>>>>> cfe/trunk/include/clang/Driver/Options.td
>>>>>>>> cfe/trunk/lib/Driver/Tools.cpp
>>>>>>>> cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>>>>>>>> cfe/trunk/lib/Lex/ModuleMap.cpp
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/include/clang/Basic/LangOptions.def
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=206027&r1=206026&r2=206027&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/include/clang/Basic/LangOptions.def (original)
>>>>>>>> +++ cfe/trunk/include/clang/Basic/LangOptions.def Fri Apr 11
>>>>>>>> 06:47:45 2014
>>>>>>>> @@ -96,6 +96,7 @@ LANGOPT(MathErrno , 1, 1, "errno
>>>>>>>> BENIGN_LANGOPT(HeinousExtensions , 1, 0, "Extensions that we
>>>>>>>> really don't like and may be ripped out at any time")
>>>>>>>> LANGOPT(Modules , 1, 0, "modules extension to C")
>>>>>>>> LANGOPT(ModulesDeclUse , 1, 0, "require declaration of module
>>>>>>>> uses")
>>>>>>>> +LANGOPT(ModulesStrictDeclUse, 1, 0, "require declaration of module
>>>>>>>> uses and all headers to be in modules")
>>>>>>>> LANGOPT(Optimize , 1, 0, "__OPTIMIZE__ predefined macro")
>>>>>>>> LANGOPT(OptimizeSize , 1, 0, "__OPTIMIZE_SIZE__ predefined
>>>>>>>> macro")
>>>>>>>> LANGOPT(Static , 1, 0, "__STATIC__ predefined macro (as
>>>>>>>> opposed to __DYNAMIC__)")
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/include/clang/Driver/Options.td
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=206027&r1=206026&r2=206027&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/include/clang/Driver/Options.td (original)
>>>>>>>> +++ cfe/trunk/include/clang/Driver/Options.td Fri Apr 11 06:47:45
>>>>>>>> 2014
>>>>>>>> @@ -607,6 +607,9 @@ def fmodules_ignore_macro : Joined<["-"]
>>>>>>>> def fmodules_decluse : Flag <["-"], "fmodules-decluse">,
>>>>>>>> Group<f_Group>,
>>>>>>>> Flags<[DriverOption,CC1Option]>,
>>>>>>>> HelpText<"Require declaration of modules used within a module">;
>>>>>>>> +def fmodules_strict_decluse : Flag <["-"],
>>>>>>>> "fmodules-strict-decluse">, Group<f_Group>,
>>>>>>>> + Flags<[DriverOption,CC1Option]>,
>>>>>>>> + HelpText<"Like -fmodules-decluse but requires all headers to be
>>>>>>>> in modules">;
>>>>>>>> def fretain_comments_from_system_headers : Flag<["-"],
>>>>>>>> "fretain-comments-from-system-headers">, Group<f_Group>, Flags<[CC1Option]>;
>>>>>>>>
>>>>>>>> def fmudflapth : Flag<["-"], "fmudflapth">, Group<f_Group>;
>>>>>>>> @@ -666,6 +669,8 @@ def fno_module_maps : Flag <["-"], "fno-
>>>>>>>> Flags<[DriverOption]>;
>>>>>>>> def fno_modules_decluse : Flag <["-"], "fno-modules-decluse">,
>>>>>>>> Group<f_Group>,
>>>>>>>> Flags<[DriverOption]>;
>>>>>>>> +def fno_modules_strict_decluse : Flag <["-"],
>>>>>>>> "fno-strict-modules-decluse">, Group<f_Group>,
>>>>>>>> + Flags<[DriverOption]>;
>>>>>>>> def fno_ms_extensions : Flag<["-"], "fno-ms-extensions">,
>>>>>>>> Group<f_Group>;
>>>>>>>> def fno_ms_compatibility : Flag<["-"], "fno-ms-compatibility">,
>>>>>>>> Group<f_Group>;
>>>>>>>> def fno_delayed_template_parsing : Flag<["-"],
>>>>>>>> "fno-delayed-template-parsing">, Group<f_Group>;
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/lib/Driver/Tools.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=206027&r1=206026&r2=206027&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/lib/Driver/Tools.cpp (original)
>>>>>>>> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Apr 11 06:47:45 2014
>>>>>>>> @@ -3469,6 +3469,14 @@ void Clang::ConstructJob(Compilation &C,
>>>>>>>> CmdArgs.push_back("-fmodules-decluse");
>>>>>>>> }
>>>>>>>>
>>>>>>>> + // -fmodules-strict-decluse is like -fmodule-decluse, but also
>>>>>>>> checks that
>>>>>>>> + // all #included headers are part of modules.
>>>>>>>> + if (Args.hasFlag(options::OPT_fmodules_strict_decluse,
>>>>>>>> + options::OPT_fno_modules_strict_decluse,
>>>>>>>> + false)) {
>>>>>>>> + CmdArgs.push_back("-fmodules-strict-decluse");
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> // -fmodule-name specifies the module that is currently being
>>>>>>>> built (or
>>>>>>>> // used for header checking by -fmodule-maps).
>>>>>>>> if (Arg *A = Args.getLastArg(options::OPT_fmodule_name)) {
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=206027&r1=206026&r2=206027&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
>>>>>>>> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Apr 11
>>>>>>>> 06:47:45 2014
>>>>>>>> @@ -1343,7 +1343,9 @@ static void ParseLangArgs(LangOptions &O
>>>>>>>> Opts.Blocks = Args.hasArg(OPT_fblocks);
>>>>>>>> Opts.BlocksRuntimeOptional =
>>>>>>>> Args.hasArg(OPT_fblocks_runtime_optional);
>>>>>>>> Opts.Modules = Args.hasArg(OPT_fmodules);
>>>>>>>> - Opts.ModulesDeclUse = Args.hasArg(OPT_fmodules_decluse);
>>>>>>>> + Opts.ModulesStrictDeclUse =
>>>>>>>> Args.hasArg(OPT_fmodules_strict_decluse);
>>>>>>>> + Opts.ModulesDeclUse =
>>>>>>>> + Args.hasArg(OPT_fmodules_decluse) ||
>>>>>>>> Opts.ModulesStrictDeclUse;
>>>>>>>> Opts.CharIsSigned = Opts.OpenCL ||
>>>>>>>> !Args.hasArg(OPT_fno_signed_char);
>>>>>>>> Opts.WChar = Opts.CPlusPlus && !Args.hasArg(OPT_fno_wchar);
>>>>>>>> Opts.ShortWChar = Args.hasFlag(OPT_fshort_wchar,
>>>>>>>> OPT_fno_short_wchar, false);
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/lib/Lex/ModuleMap.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=206027&r1=206026&r2=206027&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/lib/Lex/ModuleMap.cpp (original)
>>>>>>>> +++ cfe/trunk/lib/Lex/ModuleMap.cpp Fri Apr 11 06:47:45 2014
>>>>>>>> @@ -243,8 +243,12 @@ void ModuleMap::diagnoseHeaderInclusion(
>>>>>>>> resolveUses(RequestingModule, /*Complain=*/false);
>>>>>>>>
>>>>>>>> HeadersMap::iterator Known = findKnownHeader(File);
>>>>>>>> - if (Known == Headers.end())
>>>>>>>> + if (Known == Headers.end()) {
>>>>>>>> + if (LangOpts.ModulesStrictDeclUse)
>>>>>>>> + Diags.Report(FilenameLoc,
>>>>>>>> diag::error_undeclared_use_of_module)
>>>>>>>> + << RequestingModule->getFullModuleName() << Filename;
>>>>>>>> return;
>>>>>>>> + }
>>>>>>>>
>>>>>>>> Module *Private = NULL;
>>>>>>>> Module *NotUsed = NULL;
>>>>>>>>
>>>>>>>> Added: cfe/trunk/test/Modules/strict-decluse.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/strict-decluse.cpp?rev=206027&view=auto
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/test/Modules/strict-decluse.cpp (added)
>>>>>>>> +++ cfe/trunk/test/Modules/strict-decluse.cpp Fri Apr 11 06:47:45
>>>>>>>> 2014
>>>>>>>> @@ -0,0 +1,9 @@
>>>>>>>> +// RUN: rm -rf %t
>>>>>>>> +// RUN: %clang_cc1 -fmodule-maps -fmodules-cache-path=%t
>>>>>>>> -fmodules-strict-decluse -fmodule-name=XG -I %S/Inputs/declare-use %s
>>>>>>>> -verify
>>>>>>>> +
>>>>>>>> +#include "g.h"
>>>>>>>> +#include "e.h"
>>>>>>>> +#include "f.h" // expected-error {{module XG does not depend on a
>>>>>>>> module exporting 'f.h'}}
>>>>>>>> +#include "i.h" // expected-error {{module XG does not depend on a
>>>>>>>> module exporting 'i.h'}}
>>>>>>>> +
>>>>>>>> +const int g2 = g1 + e + f + aux_i;
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> cfe-commits mailing list
>>>>>>>> cfe-commits at cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140422/c460df1d/attachment.html>
More information about the cfe-commits
mailing list