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

[Richard Smith]

On Wed Jan 29 2014 at 4:03:06 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 29/01/2014 23:25, Richard Smith wrote:
> > On Wed Jan 29 2014 at 1:45:22 PM, Alp Toker <alp at nuanti.com
> > <mailto:alp at nuanti.com>> wrote:
> >
> >
> >     On 09/01/2014 23:26, Richard Smith wrote:
> >     > On Thu, Jan 9, 2014 at 3:18 PM, Marshall Clow
> >     <mclow.lists at gmail.com <mailto:mclow.lists at gmail.com>
> >     > <mailto:mclow.lists at gmail.com <mailto:mclow.lists at gmail.com>>>
> >     wrote:
> >     >
> >     >
> >     > On Jan 9, 2014, at 2:51 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:
> >     >
> >     >> On Thu, Jan 9, 2014 at 2:39 PM, Marshall Clow
> >     >> <mclow.lists at gmail.com <mailto:mclow.lists at gmail.com>
> >     <mailto:mclow.lists at gmail.com <mailto:mclow.lists at gmail.com>>>
> wrote:
> >     >>
> >     >> On Jan 9, 2014, at 2:20 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:
> >     >>
> >     >>> On Thu, Jan 9, 2014 at 7:25 AM, Alp Toker <alp at nuanti.com
> >     <mailto:alp at nuanti.com>
> >     >>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
> >     >>>
> >     >>> 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.
> >     >>
> >     >> A few points, in no particular order:
> >     >>
> >     >> * If we have ill-formed code in system headers, I would
> >     >> expect it to fail to compile whether the user specifies
> >     >> -Wsystem-headers or not.
> >     >>
> >     >>
> >     >> And yet in some cases we don't, because we deliberately support
> >     >> some flavors of broken system headers.
> >     >> Given that, would you want -Wsystem-headers to make such code
> >     >> produce an error or only a warning?
> >     >
> >     > Eww.
> >     >
> >     > First - is any of that code in libc++?
> >     > Second, there's two audiences that I think are affected here, and
> >     > they have different needs:
> >     > * People trying to use the system headers. These people are, in
> >     > general, unable/unwilling to change them. Giving them
> >     > warnings/errors is just noise.
> >     > * People who are working on the system headers. They need to see
> >     > the warnings.
> >     >
> >     >
> >     >>
> >     >> * If a user has a clean build, and then rebuilds with
> >     >> -Wsystem-headers, I would expect to get warnings - not
> >     >> errors. [ Isn't that what PR18327 is all about? ]
> >     >>
> >     >>
> >     >> Do you expect this because the flag starts with -W?
> >     >
> >     > Yes.
> >     >
> >     >> (Maybe that's the problem here -- it's *not* a warning flag in
> >     >> the sense that the other -W flags are. But that's not
> >     >> unprecedented -- nor is -Werror.)
> >     >
> >     >
> >     >> * There are some "interesting" language features which are
> >     >> only enabled for system headers, and cause warnings if used
> >     >> in user code.
> >     >> [ User-defined suffixes that do not start with an underscore,
> >     >> for example. ]
> >     >>
> >     >>
> >     >> This is a really great example, thanks.
> >     >
> >     > Note that putting something like this in your source file gives
> >     > you a warning;
> >     >
> >     > std::complex<long double> operator"" ilx(long double __im)
> >     > {
> >     > return { 0.0l, __im };
> >     > }
> >     >
> >     > $ clang11 junk.cpp
> >     > junk.cpp:5:31: warning: user-defined literal suffixes not starting
> >     > with '_' are
> >     > reserved; no literal will invoke this operator
> >     > [-Wuser-defined-literals]
> >     > std::complex<long double> operator"" ilx(long double __im)
> >     > ^
> >     > 1 warning generated.
> >     >
> >     >
> >     >> So it seems there are at least three different classes of errors
> >     >> that we might think about producing in system headers:
> >     >>
> >     >> 1) warnings that the user has turned into errors with -Werror or
> >     >> -Werror=foo
> >     >> 2) errors for ill-formed code that we suppress by default in
> >     >> system headers as a workaround for a system header bug
> >     >> 3) errors for code that is ill-formed outside system headers but
> >     >> valid within system headers (using reserved names, adding names
> >     >> to namespace std, that sort of thing)
> >     >>
> >     >> With -Wsystem-headers enabled, I think (1) should be an error,
> >     >> (3) should remain suppressed (not even a warning), and (2) should
> >     >> be either a warning or an error (and your first two bullets don't
> >     >> give me a clear idea of which way these cases should go).
> >     >
> >     > Agreed for #1 and #3.
> >     > And for #2, I'd like to see a warning; "This is code we wouldn't
> >     > allow outside a system header, but we do so here"
> >     >
> >     > I don't see the point of making #2 an error.
> >     > For most people (who can't fix it), it's just a hassle.
> >     > For system developers, they can read warning output just as well
> >     > as error output.
> >     >
> >     >
> >     > OK, so I think this is Alp's original patch plus a change to
> >     treat #1
> >     > as an error. As you observe, -Wuser-defined-literals is
> >     currently not
> >     > an error outside of a system header, so perhaps we have no
> >     diagnostics
> >     > in case #3 at the moment, but it's something to keep in mind for
> the
> >     > future.
> >
> >     Status update: I did implement those semantics and it doesn't make as
> >     much sense in practice as we'd hoped.
> >
> >     The problem is that many conventional flags such as a -Werror build
> or
> >     specific -Werror upgrades will now consistently cause system
> >     headers to
> >     break the build, so it's no better than where we started, and
> arguably
> >     worse.
> >
> >     As a result I'm back with proposing the original patch for PR18327
> >     as-is
> >     with more confidence. It's the only mode I've found through several
> >     iterations that adds informational/educational value in a way that
> >     users
> >     and system header developers can safely consume.
> >
> >     (There is good news though, I've got some good cleanups and fixes
> >     out of
> >     the investigation that I'll post separately, especially in the area
> of
> >     "explaining" the source of diagnostics which is currently a hack
> >     in ToT.)
> >
> >     Richard, does that work for you?
> >
> >
> > I don't think I'm following. Without your patch, -Werror
> > -Wsystem-headers was an error; why is it bad for it to stay that way?
> >
> > -Werror is the way users say "I want a broken build if you find a
> > warning". This change seems to break that.
>
> These are the complications:
>
> 1) We skip a whole lot of warnings with isInSystemHeader() early checks
> anyway. A more valid way to debug system headers for correctness would
> be to include them without -isystem, or provide a -fno-system-headers
> flag instead of making this flag try to simulate that behaviour. I don't
> like the idea of representing this flag as that or making it feel in any
> way like an ordinary warning flag because what it does is unusual.
>

I find this argument persuasive. A (-f) flag to say 'there are no system
headers' seems like a more reliable way of getting that effect than the
hit-and-miss result of -Wsystem-headers, if anyone actually wants that
behavior.

(Just telling people not to use -isystem is impractical; switching includes
between -I and isystem has various subtle consequences due to changes in
include path order, and they'd have to manually replicate the -isystem
paths we implicitly inject.)


> 2) System headers use, either intentionally or for legacy reasons, code
> that's always out of sync with the user's language mode and settings.
> libc++ uses C++11 extensions even in earlier language standards, which
> is a great idea. MSVC headers use MS extensions, which is also par for
> the course. -Werror would just make every build break without providing
> much insight.
>

That's a good point.

That said, lots of other code gets treated as system headers too, by
various projects (usually third-party dependencies, sometimes just crufty
old first-party code). If someone discovers a new warning that's finding
bugs in their code, and they also want to look for bugs in their
dependencies (including system headers and third-party code), that seems
like something we should be able to support. Being able to run with
-Wsystem-headers -Werror=my-new-favourite-warning is practically useful
today (apart from issues where -Wsystem-headers enables errors of other
kinds, which I agree should not be errors). But the new behavior isn't
*that* much worse (you can't find the diagnostics by looking for build
breaks, but you can still grep through the compiler output or whatever).

$ cat test.cpp
> #include <type_traits>
>
> $ clang -std=c++98 -Werror test.cpp
>
> $ clang -std=c++98 -Werror -Wsystem-headers test.cpp
> In file included from werror.cpp:1:
> In file included from
> /Users/alp/Projects/llvm-work/build-upstream-ninja/bin/../
> include/c++/v1/type_traits:202:
> include/c++/v1/__config:346:3: error:
> inline namespaces are a C++11 feature [-Werror,-Wc++11-extensions]
> inline namespace _LIBCPP_NAMESPACE {
> ^
>
> For these two reasons, I think any error raised by -Werror
> -Wsystem-headers would be unactionable and I don't see why anyone would
> want the mode. It's a non-goal for system headers to build cleanly
> without warnings in any configuration we support today. Indeed we
> intentionally try to highlight quirks in system headers with great
> verbosity.
>
> My intended use case with this flag is to have a way to document
> "interesting facts" about system headers, and David Wiberg seems to be a
> consumer for that use case, so I'm interested in toning it down to a
> "fun" feature that people can use to understand what's going on.
>
> So the semantics of -Wsystem-headers would be "show diagnostics in
> system headers that would otherwise be suppressed as warnings."
>
> It is quirky, but I think less so than pretending that these are
> equivalent to ordinary warning diagnostics in user code. Sometimes a
> quirky problem demands a quirky solution :-)
>

OK, I think you've sold me on this.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140130/605e5dad/attachment.html>