RFR: JDK-8211073 Remove -Wno-extra from Hotspot

[Kim Barrett]

> On Sep 29, 2018, at 1:36 PM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
> 
> On 2018-09-28 23:22, Kim Barrett wrote:
> 
>>> On Sep 24, 2018, at 4:31 PM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>> The second warning about the copy constructor is, for what I can tell, a highly valid warning and the code it warned on was indeed broken. As far as I can tell, in a derived copy constructor you should always explicitly initialize the base class.
>> I agree the copy constructor warnings are correct and indicate potentially serious bugs.
>> These copy constructor changes look correct to me.
>> 
>> The code that is being missed because of this bug is debug-only usage verification.  I think
>> bad things might happen if we C-heap allocated a ResourceObj and then copied that object.
>> Maybe we fortuitously don’t ever do that?
> If we had triggered problems we'd probably found out the issue by now, so its likely we're not doing anything that causes this to happen currently. But latent bugs like these are scary, and unnecessary, especially if we can get the compiler's help to avoid them.
>> It’s unfortunate that the only way to get these warnings from gcc seems to be via -Wextra.
> I agree, it's unfortunate. -Wextra in gcc actually means two things: a bunch of "extra" warnings, that you can turn on or off as a group only by enabling or disabling -Wextra, and a set of separate warnings that this option also turns on by default. The latter is more of a convenience to get a set of warnings that the gcc authors recommend for more in-depth warnings. Since they can be individually disabled, we don't need to care much about them.
> 
> In regard to your previous mail, there has not been much change in the scope of -Wextra. Between gcc 4.8 and 7.3, no changes were made in the first part (the "extra" warnings), and only four more warnings were added to the second part (-Wcast-function-type, -Wimplicit-fallthrough=3, -Wredundant-move and -Wshift-negative-value), all of which can be turned off if we don't want them.
> 
> In general, we try to make the product compile without warnings on common platforms, but as you say, unless you use one of the compilers that are regularly used (at Oracle or elsewhere), there's a risk of triggering new warnings. However, using configure --disable-warnings-as-errors makes the build pass anyway, and is already commonly in use by those building on more exotic platforms or compiler versions.
> 
> I would have preferred to have individual warnings to enable, but gcc has not moved all warnings from -Wextra to individual warnings (new warnings, though, have all been given individual names). Since the warnings, as you agree, can find actual bugs, I think it's worth the hassle to enable -Wextra. (We already do that for all native code in OpenJDK, except hotspot, btw.)

gcc8.2 adds -Wcast-function-type, which *might* cause us problems.  We
play some interesting games with function types in some places, and I
don't know if they'll trigger this.  We can turn it off explicitly if
it is a problem (or perhaps preemptively).

I went through all the associated warnings for gcc7.3 and categorized
them.  Some are just not interesting or relevant to us.  I think these
include

-Wclobbered
-Wignored-qualifiers
-Wmissing-field-initializers 
-Wmissing-parameter-type
-Wold-style-declaration
-Woverride-init 
Ambiguous virtual bases.
Subscripting an array that has been declared register
Taking address of a variable declared register

Some we are already using:
-Wsign-compiler
-Wtype-limits
-Wuninitialized

Some I think we should never enable (so need to explicitly disable if
adding -Wextra):
-Wempty-body
-Wshift-negative-value
-Wunused-parameter

Perhaps provide some small benefit:
-Wunused-but-set-parameter (but does nothing without -Wunused or -Wall)
Relational compare of pointer against integer 0

-Wimplicit-fallthrough=3 requires a structured comment or C++17
feature to quiet the warning in code that is using the feature.
Provides some benefit.  I'm surprised there aren't any warnings from
this. Intentional fall-through is a pretty common idiom.

Enumerator and non-enumerator as results of conditional expression.
Has a well-defined meaning, and used in a number of places.  Some of
those might be artifacts of some constants being defined using enum
members rather than static const members.  Problematic because of
valid uses and can't be turned off.

A base class is not initialized in the copy constructor of a derived
class. Indicates a real bug, of which we have 3(?) occurrences, all of
which should be fixed. But copy constructors are relatively rare in
HotSpot, at least currently. Much of the benefit could be obtained by
occasionally doing a build with this enabled and warnings-are-errors
disabled and looking for these warnings.

I think the benefit we get from detecting future occurrences of the
copy constructor bug is not really worth the current cost of "fixing"
the occurrences of conditional expressions with enumerators.  I might
feel differently if a pass were made through those to replace uses of
enum with static const members.

In summary, I think we shouldn't enable -Wextra. I think the current
cost/benefit tradeoff is actually negative. One warning not
individually controlled requires ugly workarounds or possibly
non-trivial code changes to address, and I really dislike the ugly
workarounds.

If we do enable it, I think there are a number of specific warnings
that ought to be explicitly disabled, either temporarily or
permanently.