[PATCH] Add a new flag -fmodules-require-modular-includes

[Richard Smith]

On Thu, Apr 17, 2014 at 3:45 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Apr 14, 2014, at 6:59 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> Sure, this sounds like a good idea to me.
>
> On Mon, Apr 14, 2014 at 4:24 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>> Fair point. In that case how about we add all -Werror= and -Wfatal-error=
>> options to the module hash? That way we are consistent about any diagnostic
>> options that would prevent us from building the module.  We don’t need/want
>> to include regular warning flags, since we won’t get warnings anyway after
>> the module is built the first time.  I suppose we would want to track only
>> the canonical (last) options and sort them to avoid spurious hash changes.
>>
>> Then we could replace -f with -Werror= for this option.
>>
>
> On further consideration, this doesn’t seem like an obviously good
> solution.  Adding all of the canonical diagnostics-as-error flags into the
> module hash leads to a proliferation of module caches and reduces sharing.
>  In particular, when -Werror is specified it means that any difference in
> -W flags will prevent module sharing.  This is bad for sharing of system
> modules, which are largely unaffected by -W settings anyway, since
> -Wsystem-headers is usually not enabled.
>

That seems fixable: if a diagnostic group doesn't control any diagnostics
marked WarningShowInSystemHeader, it shouldn't be included in the hash of a
system module.

====
>
> An alternative solution to diagnostics-as-errors in modules is to record
> all of the diagnostics that we attempt to report (even if they are mapped
> as Ignored) into the module's pcm file (the module header, in the
> terminology we used before), and then at load time check whether these have
> been mapped to errors.  If so, we can rebuild the module to get the
> appropriate diagnostics.  This should keep the status quo for sharing of
> modules while allowing us to fail to build when we need to.
>
> Now this is somewhat complicated by the fact that many of our potentially
> reachable diagnostics never hit Diag.Report(), because of patterns like
> this:
>
> if (D.getDiagnosticLevel(DiagID) != Ignored) {
>    … expensive checking ...
>    D.Report(DiagID, …);
> }
>
> However, we can get around this by passing -Weverything to the
> DiagnosticsEngine, and then use a filtering diagnostic consumer to avoid
> emitting all of the unwanted warnings during a module build.  With
> -Weverything, all of the diagnostics should reach Report (excepting cases
> overridden by diagnostic pragmas, but we want to ignore those anyway),
> allowing us to collect them for saving in the pcm file.  This increases
> compile time for the module somewhat (because of expensive warning
> analysis), but improves sharing of the result compared to putting options
> in the hash.
>
> ====
>
> With that said, I would like to separate the issue of solving all of the
> diagnostics-as-errors problem from getting this specific diagnostic to
> work.  Would it be okay to commit this option as a language option for now
> and plan to move it to -W once the general problem of -Werror is fixed?  Or
> if that is really unpalatable, I could still move this to -W, but make it a
> special case and add it to the module hash (again, until the more general
>  problem is fixed).
>

It's unpalatable that we already have a language option that does this. I
really don't want us to grow a second one.

I'm also still not clear on why you think this diagnostic is different from
any other -Werror= diagnostic: why would it be OK to have this one cause us
to build another module variant, but not have the same behavior for other
-Werror= diagnostics?

> On Apr 14, 2014, at 11:24 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> I don't see why this is a special case. We have that same issue with
>> *all* warning flags.
>>
>> On Mon, Apr 14, 2014 at 9:22 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>
>>> The primary reason I didn’t go that route is that this flag should show
>>> up in the module hash so that we don’t load modules that depend on
>>> non-modular content simply because they are already built.
>>>
>>> Ben
>>>
>>> On Apr 13, 2014, at 6:15 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>> Have you considered making this be a normal warning flag, instead of a
>>> language options and an error?
>>>
>>>
>>> On Thu, Apr 10, 2014 at 6:43 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>>
>>>> As suggested off-list, this updated patch only affects frameworks for
>>>> now, although the intent is to include all modules in the future.  It also
>>>> has small fix for submodules including non-modular content and files that
>>>> are nested inside umbrella directories.
>>>>
>>>> Ben
>>>>
>>>>
>>>>
>>>>
>>>> On Apr 8, 2014, at 5:09 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>>>>
>>>> > When set, the new flag enforces that all of the files included by a
>>>> module are themselves part of a module, or are explicitly excluded by some
>>>> module. This will not affect headers that are part of the module being
>>>> built, nor files included outside of the module build (e.g. in an
>>>> implementation file with -fmodule-name set).
>>>> >
>>>> > Ben
>>>> >
>>>> >
>>>> <non-modular-includes.patch>_______________________________________________
>>>> > 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/20140417/2ea4c825/attachment.html>