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

[Coleen Phillimore]

On 11/5/14, 2:56 PM, Stefan Karlsson wrote:
> 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 don't think a "return 0;" line after ShouldNotReachHere(); which only 
knowing the contents of this call, ie that it doesn't return, is worse 
than some #pragma doesnt-return macro.   The latter is more noise and 
distraction to me.

I agree with Stefan.  I don't think this should stop this change. This 
change is an improvement.

Coleen

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