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

Fri Apr 18 14:09:18 PDT 2014

On Fri, Apr 18, 2014 at 11:46 AM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Apr 17, 2014, at 4:02 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> 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.
>
>
> Are you suggesting putting these flags into the hashed *name* of the pcm
> file (like I did for module map file path), as opposed to the module cache
> directory hash?  I was assuming that we would put this in the module cache
> directory hash.  We have no way today to use modules from more than one
> cache directory, so we couldn’t import system modules from non-system
> modules.  Of course that could be changed.
>

Yeah, this would need to go in the hashed name to make this approach work.

However, on reflection I think even this is overkill -- there's no need to
have both 'module X without -Werror=foo' and 'module X with -Werror=foo' in
the module cache; if both modules build without errors, they should be
identical. So, another possibility:

When we load a module, if we find that it's up-to-date, check if there's
any relevant -Werror flags in the current set of warning flags that were
not present when the module was built. If there are, rebuild the module
with the union of its current -Werror flags (which we already know won't
produce any diagnostics) and ours. (If it's not up-to-date, we should only
use our own -Werror flags.)

====
>>
>> 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.
>
>
> Okay.
>
>
> 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?
>
>
> There is no theoretical reason that I am aware of to differentiate this
> with other -Werror options.  But this is the one I want to make work right
> now and having one more variant (and temporarily at that, if we decide to
> put this info into the pcm instead of the hash) is much less than N.
>

If this isn't a special case, it shouldn't get special-case handling. (I
also don't really buy the implied claim that building a module once per
/configuration/ of code using that module is problematic. The cost of
building a module is paid off once you've built only a handful of
translation units that use the module.)

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/20140418/11efe20c/attachment.html>