RFR: 8062808: Turn on the -Wreturn-type warning

[Stefan Karlsson]

On 2014-11-05 19:34, Kim Barrett wrote:
> On Nov 5, 2014, at 7:22 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>> Hi all,
>>
>> I propose that we turn on the -Wreturn-type warning when compiling HotSpot with GCC.
>>
>> This will help us catch missing return statements earlier in the development cycle.
>>
>> http://cr.openjdk.java.net/~stefank/8062808/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8062808
> I’ve only skimmed this and not really reviewed, but I really dislike insertion of purportedly unreachable “return” statements to silence compiler warnings.  I have a similar dislike for “if (check for bad case) { non-returning error processing } /* no else */ …” to disable such warnings by avoid an apparent terminating control flow w/o return at the end of the error processing.  There’s got to be a better way…

I understand, but that's what you'll find if you look at the shared 
code. I only added a few more places, where our other compilers didn't 
complain about this or the code wasn't compiled with those compilers. 
With that said, I'm all for cleaning this up, but it's a pretty large 
undertaking that I don't think should prevent the usage of -Wreturn-type.

>
> I know that gcc/clang/MS all have annotation mechanism to mark code unreachable or functions as no-return.

My first implementation of thisused notreturn annotations on 
gcc/clang/MS, but since we need to support other compilers that don't 
have this annotation we can't really take advantage of the annotation to 
fix this problem throughout our code base. We would still have all these 
constructs that you've pointed out, except for the two I added.

>   I’d rather see something like that added and used before we turn on -Wreturn-type, rather than littering / contorting code to avoid that warning.

I don't agree. The code is already littered with these kind of 
constructs, so that I add a couple of similar returns shouldn't be a 
show-stopper for this warning, IMHO.

>   And compilers may generate better code sometimes when such are used.

A benefit of using the noreturn annotation would be to be able to turn 
on -Wuninitialized and get away with constructs like this:
int a;
switch (v) {
case 1: a = x; break;
case 2: a = y; break;
default: ShouldNotReachHere();
}
use(a);

I have a patch were I started testing this, but there are a number of 
places were the compiler doesn't manage to infer that all paths taken 
will initialize the variable and we have to change the code to make it 
easier for the compiler to understand it. This will probably help with 
the readability of the code, so that is probably not something negative.

>
> And I say that having just last week wasted half a day debugging what turned out to be a small refactoring that left a code path without a return.

I've asked around and I've heard of a hand-full of HotSpot developers 
that has been bitten by this. :)

thanks for looking at this,
StefanK

>
>