[PATCH] PR18327: -Wsystem-headers introduces build errors

Richard Smith richard at metafoo.co.uk
Thu Jan 9 14:20:27 PST 2014 

On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <alp at nuanti.com> wrote:

>
> On 09/01/2014 03:29, Richard Smith wrote:
>
>> On Wed, Jan 8, 2014 at 5:53 PM, David Wiberg <dwiberg at gmail.com <mailto:
>> dwiberg at gmail.com>> wrote:
>>
>>     2014/1/8 Richard Smith <richard at metafoo.co.uk
>>     <mailto:richard at metafoo.co.uk>>:
>>
>>     > On Tue, Jan 7, 2014 at 8:33 PM, Alp Toker <alp at nuanti.com
>>     <mailto:alp at nuanti.com>> wrote:
>>     >>
>>     >>
>>     >> On 08/01/2014 04:21, Argyrios Kyrtzidis wrote:
>>     >>>
>>     >>> On Jan 7, 2014, at 7:56 PM, Alp Toker <alp at nuanti.com
>>     <mailto:alp at nuanti.com>> wrote:
>>     >>>
>>     >>>> On 08/01/2014 01:48, Argyrios Kyrtzidis wrote:
>>     >>>>>
>>     >>>>> On Jan 6, 2014, at 1:47 PM, Richard Smith
>>     <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>
>>     >>>>> <mailto:richard at metafoo.co.uk
>>
>>     <mailto:richard at metafoo.co.uk>>> wrote:
>>     >>>>>
>>     >>>>>> One view on this is simply: -Wsystem-headers means "don't
>>     give system
>>     >>>>>> headers special treatment when emitting diagnostics”.
>>     >>>>>
>>     >>>>> This is it exactly. “Treat all headers like normal headers"
>>     >>>>
>>     >>>> We don't have a flag to treat all headers as normal headers
>>     at the
>>     >>>> moment. It'd be very simple to implement compared to
>>     -Wsystem-headers which
>>     >>>> somewhat intricate.
>>     >>>
>>     >>> Could you elaborate, AFAICT '-Wsystem-headers' is treated
>>     specially, it's
>>     >>> outside the diagnostic group machinery, and acts essentially
>>     as a flag.
>>     >>> If you'd like to have something like '-fwarn-on-system-headers' or
>>     >>> something instead, that's another discussion, but as far as
>>     the PR is
>>     >>> concerned I don't see why we need to change what
>>     -Wsystem-headers is
>>     >>> currently doing.
>>     >>
>>     >>
>>     >> PR18327 reports a valid corner-case bug in the way a very small
>>     number
>>     >> (around 8 in total) diagnostics are upgraded from
>>     warnings/extensions to
>>     >> errors, and then were forgetting to downgrade them back to
>>     warnings as all
>>     >> the other diagnostics are seen. So it's just an implementation
>>     detail that
>>     >> was leaking and manifesting as errors in a context where it
>>     should have been
>>     >> impossible.
>>     >
>>     >
>>     > These diagnostics are set up as 'errors that we suppress in
>>     system headers'
>>     > (they're suppressed because they actually happen in some system
>>     headers).
>>     >
>>     > If the viewpoint is that '-Wsystem-headers' means 'don't suppress
>>     > diagnostics in system headers', then issuing errors in these
>>     cases seems
>>     > correct to me. If the viewpoint is that '-Wsystem-headers' means
>>     'warn on
>>     > problems in system headers', then issuing warnings, not errors,
>>     in these
>>     > cases seems correct. It depends on what the user of the flag
>>     intends it to
>>     > mean.
>>     If an error has been downgraded for system headers I don't think it
>>     makes sense if -Wsystem-headers (which in the user's manual is
>>     described as "Enables warnings from system headers.") causes the build
>>     to fail. How is a system library developer supposed to check for
>>     warnings if it isn't possible to enable warnings without breaking the
>>     build?
>>
>>
>> They're supposed to fix their code so it's valid C++ =) (and they can use
>> the warning flag name we list, to turn the error off).
>>
>>     > I think the former option makes a little more sense for system
>>     header
>>     > developers -- if their system headers contain ill-formed code,
>>     they probably
>>     > want to know about that with more urgency than if their system
>>     headers
>>     > merely contain dubious code. But we still don't know what the
>>     person who
>>     > filed the original PR was trying to do, and maybe there's a use
>>     case where
>>     > the latter view makes more sense.
>>     The use case for me was that I was looking at a bug report and when
>>     reducing a test case I ended up finding a problem in lib\Headers. When
>>     enabling -Wsystem-headers the build failed early instead of completing
>>     and producing a warning to point me in the right direction.
>>
>>
>> OK, and because the error caused your build to fail, it was harder for
>> you to perform the reduction? That's an interesting situation, but it seems
>> pretty easy to work around once you know what's happening. I'm still on the
>> fence here.
>>
>
> Hi Richard,
>
> I've taken a step back to look at this in context.
>
> David Wiberg's end-user use case appears entirely reasonable to me,


I agree. He can achieve his use case with -Wsystem-headers
-Wno-error=invalid-constexpr.


> and there's a strong expectation that -Wsystem-headers shouldn't introduce
> errors in valid system header code.


I don't think this has been demonstrated. We have only one report of a user
being surprised here, and at least one Clang developer believes the current
behavior is appropriate.


> It's not fair on the user or the system header maintainers to bring up
> unexpected errors with this flag.
>

I don't think this is clear. If the code in the system header is
ill-formed, I would expect the system header maintainers to be *grateful*
that we flag it as an error and not just as a warning. But I'll wait to
hear what they have to say about that.


> In this case, the problem was that the flag was inadvertently disabling a
> system header workaround that you introduced, and that's not really the
> purpose of the flag to disable compatibility features. It does appear just
> to be a bug that my patch resolves.
>
> We absolutely should have a -fno-system-headers that simply disables
> special handling of system headers and it'll be an invaluable tool for
> standard library developers. But this is not the right flag for that.
>
> -Wsystem-headers should have minimal impact beyond doing "what it says on
> the tin" so that interested developers can explore the system header
> workarounds and warnings safely without risking a broken build.
>
> Anything else will draw unwanted problem reports against the standard
> library implementation.
>
> For these reasons I'm happy with my original patch as a conservatively
> correct step in the right direction.
>
> Some background: I've realized after discussions with the libc++
> developers that a small change like this can be a headache for system
> header developers. This has made me very conscious that we take care to
> preserve any workarounds and not silently upgrade them to hard errors in
> user code that ordinary developers will encounter.
>
> So I see it as part of the unspoken contract we have with the libc++ team
> to fix this in the conservative way I outlined in PR18327, such that the
> flag doesn't introduce errors for code that's otherwise accepted.
>
> Alp.
>
>
>
>      >> Remember that this flag doesn't control the very many
>>     isInSystemHeader()
>>     >> and isInSystemMacro() checks that happen earlier than the
>>     diagnostic
>>     >> machinery. A -fno-system-headers flag to just disable the whole
>>     system
>>     >> header machinery would be separately useful though, agreed.
>>     >
>>     >
>>     > I agree. If we want this flag to mean 'there are no system
>>     headers', it does
>>     > not go far enough.
>>     >
>>     >>
>>     >>
>>     >> Alp.
>>     >>
>>     >>
>>     >>
>>     >>
>>     >>
>>     >>>
>>     >>>> Alp.
>>     >>>>
>>     >>>>>> That would seem to make perfect sense to people developing
>>     system
>>     >>>>>> headers, and is our current behavior. What is the use case
>>     that leads to
>>     >>>>>> enabling -Wsystem-headers but not wanting that to lead to
>>     errors? PR18327
>>     >>>>>> doesn't make that obvious.
>>     >>>>>
>>     >>>>> Not sure I’m following that report, if one doesn’t like that
>>     that
>>     >>>>> diagnostic is by default mapped to an error, maybe map it to
>>     a warning on
>>     >>>>> the command-line or discuss whether it should not be mapped
>>     to error by
>>     >>>>> default ?
>>     >>>>> I don’t see a need to complicate what -Wsystem-headers does.
>>     >>>>>
>>     >>>> --
>>     >>>> http://www.nuanti.com
>>     >>>> the browser experts
>>     >>>>
>>     >>
>>     >> --
>>     >> http://www.nuanti.com
>>     >> the browser experts
>>     >>
>>     >
>>     >
>>     > _______________________________________________
>>     > cfe-commits mailing list
>>     > cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>     > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>     >
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/4aa2f2ed/attachment.html>

More information about the cfe-commits mailing list