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

Fri Apr 18 14:47:19 PDT 2014

On Apr 18, 2014, at 2:09 PM, Richard Smith <richard at metafoo.co.uk> wrote:

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

Sounds great!  In hindsight, this seems like the obvious refinement of putting the diagnostics state into the hash.  I will attempt to make it so.

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

I won’t press the point since we seem to have a good direction for a general solution (yay).

Thanks for helping iron this out,

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/20140418/36545a09/attachment.html>