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

[Magnus Ihse Bursie]

Kim,

Thank you for your detailed analysis! Comments inline...

3 okt. 2018 kl. 00:18 skrev Kim Barrett <kim.barrett at oracle.com>:

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

I agree that some, like clobbered, is not relevant (but I think that also only applies to C code..?). In general, it doesn't hurt though, to turn on warnings that are unlikely to be triggered. I realize it also helps little to nothing, but once again, if this is the price we have to pay for catching some real problems, it's a small price indeed. 

OTOH, I don't really agree that old-style-declaration is irrelevant. At least in the JDK libraries there are multiple places where non-prototype declarations are made, and apart from the style issue, it's a bug waiting to happen if the function change its signature. I don't remember if this is the case in Hotspot too, but it wouldn't surprise me, if we don't check for it. 

> 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

I agree that unused-parameter is worthless. I think we should disable it globally, if that's not already done. 

empty-body is mostly good at catching style issues, like double semicolons. It's not a big deal, but it also never hurts to keep up good code quality. I have not seen it make any false positives.

shift-negative-values is a bit beyond me, but as I understand it, it warns about a behavior that has changed recently, or will be changed, in the standards, to become undefined. That sounds to me like something that it would be better to act preemptively on. But then again, I might have misunderstood things. If so, we can definitely turn it off. 

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

At level 3, the "structured" comment is just something like a line like this:

// fallthrough

Most, maybe all, fallthroughs in the hotspot code already does that. I think it's a reasonable requirement to show that the fallthrough is intentional. If you think it's too strong, we can lower it to level 1, which just requires a comment, with whatever text (or even empty, I think).

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

Agree, this is the Achilles heel really. It's too bad it can't be turned off. :( The remaining code changes in my patch (apart from the copy constructor) are all due to this.

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

Only this is never going to happen. :( In general, I really much prefer for issues that can be detected automatically to be detected automatically. One-shot checks are unlikely to happen, and if done seldom enough will only accumulate so much "cruft" that it's too much work and too boring work for anyone to bother doing. 

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

So I think this is what it really boils down to. Just to summarize/repeat what you said, so I'm sure we understand each other correctly:

There's an upside to enabling Wextra; we know already that it found some real bugs, but the real benefit is all future potential bugs it will prohibit, and that's hard to quantify. On the other hand, there's a downside; and that's the conditional enum warnings, whose fixes are ugly. But, if the code was cleaned up, so those enum fixes were no longer ugly, then it would be a net positive to turn on Wextra.

I think that's a reasonable solution for now. If we agree on this, I'll open a new bug with just the copy constructor fixes, and a enhancement request for replacing the enums with static const, and making the current bug ("turn on Wextra") dependent on that. 

I just want to add a final remark about compiler warnings. When I started the current work of normalizing the set of warnings in the entire OpenJDK, I was amazed at what low level of warnings that was used for gcc/clang in hotspot. In all the JDK native libraries, -Wall and -Wextra were on by default. In hotspot, not even -Wall were turned on! I think what have sort of "saved" you is that Microsoft CL was running with /W3, which is roughly equivalent to -Wall -Wextra. So most of the shared code compiled fine when adding -Wall -Wextra, but there was a lot of new warnings in non-Windows code. Of course, /W3 and -Wall -Wextra does not cover exactly the same set of warnings, nor implement similar warning checks identically, hence the current open set of warnings in shared code. 

I made a long list of warnings that are disabled due to this, but I think it would be a good idea to tackle them one by one. By a quick glance, most of them (but surely not all) seemed to point to real problematic code; if not buggy so at least bad style. 

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

Yes, definitely. Had Wextra been *only* a predefined set of other named warnings, we wouldn't have needed to have this discussion. But since it works the way it does, it means that when we turn it on, we have to turn off the included warnings that we do not want.

/Magnus