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

[Kim Barrett]

> On Oct 4, 2018, at 9:14 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
> 
> 3 okt. 2018 kl. 00:18 skrev Kim Barrett <kim.barrett at oracle.com>:
> 
>> 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
>> 
>> […]
> 
> 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. 

I made this list for completeness of discussion.  I think these
options should carry no weight and should just be ignored in the
decision making process.

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

Regarding -Wold-style-declaration, remember that we're discussion a
change to HotSpot, where this isn't relevant.

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

-Wempty-body doesn't catch empty semi-colons (which are permitted in
C++11 and later, and no compiler I know of complains about them in
C++98 mode, because they arise too often with conditional compilation
and macros and such).

There is also a well-known macro idiom involving empty if bodies, e.g.

  if (!<test>) {} else <expr-if-test-true>

to avoid dangling else.  Since you didn't run into any occurences of
this warning, we're probably not using that idiom right now.  But
there's nothing wrong with it and I've used it myself in the past.  It
could have been used in some of the UL macros, but a different idiom
was used there.  (The two are semantically mostly equivalent, the
differences being fairly subtle.  If I'd written these macros they
would probably be using the "if" style.)

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

I mis-remembered the situation with -Wshift-negative-value; we
probably do want that turned on once we've eliminated uses (and there
are some).  C89/C++98 didn't specify the behavior in that case.
C99/C++11 clarified that it is explicitly UB.


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

I'm concerned that other compilers / checkers might want other
comments, and one might end up with a bit of a mess because of that.
If Visual Studio (for example) has a similar warning option, we might
want to turn that on too.

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

That seems like a good plan to me.

> 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 mostly agree with this, though the version to version changes in the
content of these umbrella warning sets is a concern.

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

Yes.  For example, Mikael V has a patch set to enable -Wunused-variable.
It’s unpleasantly large and in some parts intrusive.  And there is 
https://bugs.openjdk.java.net/browse/JDK-8135181
Re-enable '-Wconversion' for GCC 4.3 and later
which would also be huge and intrusive.