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

[Ben Langmuir]

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.

====

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).

Ben


> 
> Ben
> 
> 
> 
> 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/2225c4b2/attachment.html>