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

[Magnus Ihse Bursie]

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

/Magnus

>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8211073
>> WebRev: http://cr.openjdk.java.net/~ihse/JDK-8211073-remove-Wno-extra-from-hotspot/webrev.01
>>
>> /Magnus
>>
>> [1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
>