Bug 50871 - libstdc++ should be built with -Wpedantic and/or -Wsystem-headers
Summary: libstdc++ should be built with -Wpedantic and/or -Wsystem-headers
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.6.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: accepts-invalid
Depends on:
Blocks:
 
Reported: 2011-10-25 22:28 UTC by Jonathan Wakely
Modified: 2017-12-01 20:25 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-04-04 00:00:00


Attachments
patch to add -Weh-mismatch to handle this case (1.01 KB, patch)
2017-11-30 15:34 UTC, Jason Merrill
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2011-10-25 22:28:50 UTC
Preprocessed source, note first line:

# 1 "/" 3

class condvar
{
public:

  condvar() noexcept;
  ~condvar() noexcept;

};

condvar::condvar() = default;
condvar::~condvar() = default;


This should be rejected because the explicitily-defaulted definitions are missing "noexcept" but it compiles without error:

$ g++ -std=gnu++0x  cv.ii -c
$


If the first line is removed it is rejected:

cv.ii:11:18: error: declaration of ‘condvar::condvar()’ has a different exception specifier
cv.ii:6:3: error: from previous declaration ‘condvar::condvar() noexcept (true)’
cv.ii:12:19: error: declaration of ‘condvar::~condvar()’ has a different exception specifier
cv.ii:7:3: error: from previous declaration ‘condvar::~condvar() noexcept (true)’
Comment 1 Paolo Carlini 2011-10-26 10:14:55 UTC
Well, the error itself is protected by ! DECL_IN_SYSTEM_HEADER (old_decl). Thus I guess the point is whether we want to do something special for C++11 here, like errors involving defaulted definitions or noexcept itself. Maybe Jason has something to suggest..
Comment 2 Jonathan Wakely 2011-10-26 10:40:30 UTC
Ah of course it's a system header thing, but -pedantic would have caught it.

This let a bug slip through in the library, which I fixed with http://gcc.gnu.org/viewcvs/trunk/libstdc%2B%2B-v3/src/condition_variable.cc?r1=180454&r2=180453&pathrev=180454 
It would be nice if the compiler had caught it. I assume the condition is there to handle badly-written system headers, but I don't think we want to allow mismatched exception spec in libstdc++ itself.
Comment 3 Paolo Carlini 2011-10-26 11:31:07 UTC
I know. -pedantic works indeed. Really, changing this would be easy, if we wanted, maybe it's just something from the past? Jason should know...
Comment 4 Jason Merrill 2011-10-26 13:37:23 UTC
Are the libstdc++ headers treated as system headers when building the library itself?  If so, that seems like the thing to fix.
Comment 5 Jonathan Wakely 2011-10-26 13:46:45 UTC
Yes they are. Not doing that would be a good idea
Comment 6 Paolo Carlini 2011-10-27 13:17:15 UTC
Couldn't we start on this by simply building the library with -pedantic? I think this something we can di now, and would have catched a similar thinko of mine a couple of weeks ago
Comment 7 Jonathan Wakely 2011-10-27 13:31:17 UTC
It would work for my original case, yes.  There are other cases where the warning is not a pedwarn, but is just suppressed by system_header while building the lib.

But it occurred to me that we currently rely on system_header to suppress warnings about using variadic templates in c++98 mode
Comment 8 Paolo Carlini 2011-10-27 13:34:31 UTC
Yes, and I'm afraid we rely on it even more, not something we can do today.
Comment 9 Jason Merrill 2011-11-10 22:01:14 UTC
Changing component.
Comment 10 Jonathan Wakely 2011-12-03 15:34:53 UTC
It occurs to me we could use a diagnostic pragma to disable warnings about variadic templates in our headers, instead of marking them as system headers.
That wouldn't work currently because the variadic templates warning is [enabled by default] so there's no option to disable it.
Comment 11 Jonathan Wakely 2014-04-04 17:05:54 UTC
This bug hides the fact that we've apparently always been missing exception specs on the operator== and operator!= for std::allocator.

Clang noticed this:

/home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/20_util/headers/memory/synopsis.cc:27:8: error: exception specification in declaration does not match previous declaration
/home/jwakely/src/gcc/gcc/libstdc++-v3/testsuite/20_util/headers/memory/synopsis.cc:29:8: error: exception specification in declaration does not match previous declaration
2 errors generated.

I'll look into using -pedantic when building the library and maybe for some of the testsuite.
Comment 12 Jonathan Wakely 2014-06-24 15:00:27 UTC
I've tried adding -Wpedantic to the WARN_FLAGS used to build libstdc++, but it produces a lot of noise due to the use of extern/inline explicit instantiations for templates, some of which are in .cc files not headers, so don't benefit from -Wsystem-headers.

Unlike -Wno-long-long and -Wno-variadic-macros (both needed to stop too many pedantic warnings) I don't think there's any way to use -Wpedantic without getting warnings about the extern templates.  We could move the affected sources from src/c++98/ to src/c++11/ and compile as C++11 but that's probably a bigger change than we want to do right now.

So I'm just going to try to remember to occassionally build using
  --enable-cxx-flags="-Wpedantic -Wno-long-long -Wno-variadic-macros" 
and check there are no new problems.
Comment 13 Jason Merrill 2017-11-30 15:34:01 UTC
Created attachment 42754 [details]
patch to add -Weh-mismatch to handle this case

Since the main concern seems to be EH mismatch, how about this?
Comment 14 Jonathan Wakely 2017-11-30 18:09:00 UTC
It's better than what we have today, but I'm not sure it's where we want to get to in the long term.

I've been trying a patch  that allows the system_header pragma to be disabled when building and testing. This causes testsuite failures due to warnings that can't be controlled by options or diagnostic pragmas:

/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/bits/std_abs.h:40: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cstdlib:77: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/fenv.h:38: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:47: error: #include_next is a GCC extension
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/complex:1951: warning: floating point suffix 'if' shadowed by implementation
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/complex:1959: warning: floating point suffix 'i' shadowed by implementation
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/complex:1963: warning: integer suffix 'i' shadowed by implementation
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/complex:1967: warning: floating point suffix 'il' shadowed by implementation
/home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/complex:1971: warning: integer suffix 'il' shadowed by implementation
Comment 15 Jason Merrill 2017-11-30 18:36:09 UTC
(In reply to Jonathan Wakely from comment #14)
> It's better than what we have today, but I'm not sure it's where we want to
> get to in the long term.
> 
> I've been trying a patch  that allows the system_header pragma to be
> disabled when building and testing.

How about using 

#pragma GCC diagnostic warning "-Wsystem-headers"

instead?

> This causes testsuite failures due to
> warnings that can't be controlled by options or diagnostic pragmas:
> 
> /home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:
> 47: error: #include_next is a GCC extension

Maybe leave off -Wpedantic for now.

> /home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/
> complex:1951: warning: floating point suffix 'if' shadowed by implementation

This seems like a real issue.  Perhaps for C++14 and up we should disable the built-in complex suffixes.
Comment 16 Jonathan Wakely 2017-11-30 20:41:59 UTC
(In reply to Jason Merrill from comment #15)
> (In reply to Jonathan Wakely from comment #14)
> > It's better than what we have today, but I'm not sure it's where we want to
> > get to in the long term.
> > 
> > I've been trying a patch  that allows the system_header pragma to be
> > disabled when building and testing.
> 
> How about using 
> 
> #pragma GCC diagnostic warning "-Wsystem-headers"
> 
> instead?

That doesn't work well, see PR 80472
 
> > This causes testsuite failures due to
> > warnings that can't be controlled by options or diagnostic pragmas:
> > 
> > /home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/cmath:
> > 47: error: #include_next is a GCC extension
> 
> Maybe leave off -Wpedantic for now.

Yep. It would be nice if we had __include_next, or could add __extension__ to avoid those warnings.

> > /home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/
> > complex:1951: warning: floating point suffix 'if' shadowed by implementation
> 
> This seems like a real issue.  Perhaps for C++14 and up we should disable
> the built-in complex suffixes.

See PR 79228
Comment 17 Jonathan Wakely 2017-11-30 20:44:50 UTC
(In reply to Jonathan Wakely from comment #16)
> (In reply to Jason Merrill from comment #15)
> > (In reply to Jonathan Wakely from comment #14)
> > > It's better than what we have today, but I'm not sure it's where we want to
> > > get to in the long term.
> > > 
> > > I've been trying a patch  that allows the system_header pragma to be
> > > disabled when building and testing.
> > 
> > How about using 
> > 
> > #pragma GCC diagnostic warning "-Wsystem-headers"
> > 
> > instead?
> 
> That doesn't work well, see PR 80472

Oh, did you mean putting that in the libstdc++-v3/src/*/*.cc sources, and tests, rather than in the headers?

That could work as long as it doesn't also enable warnings for C library headers, which we generally can't fix or avoid.
Comment 18 Jason Merrill 2017-12-01 20:25:48 UTC
(In reply to Jonathan Wakely from comment #16)
> (In reply to Jason Merrill from comment #15)

> > > /home/jwakely/build/powerpc64le-unknown-linux-gnu/libstdc++-v3/include/
> > > complex:1951: warning: floating point suffix 'if' shadowed by implementation
> > 
> > This seems like a real issue.  Perhaps for C++14 and up we should disable
> > the built-in complex suffixes.
> 
> See PR 79228

Fixed.