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

[Stefan Karlsson]

On 2014-11-05 21:26, Coleen Phillimore wrote:
>
>
> 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.

Actually, the noreturn experiment/patch hides it within 
ShouldNotReachHere(); so you wouldn't get any line noise.

This is done by doing something like this:

// globalDefinitions.hpp
WITH_NORETURN_ATTRIBUTE(void noreturn_function());

// globalDefinitions.cpp
void noreturn_function() {
   // Make sure that this function never returns.
   while (true) {
     os::naked_short_sleep(10);
   }
}

// globalDefinitions_gcc.hpp
#define WITH_NORETURN_ATTRIBUTE(function) function ___attribute((noreturn))

// globalDefinitions_
#define ShouldNotReachHere() \
do { \
   report_should_not_reach_here(__FILE__, __LINE__); \
   BREAKPOINT; noreturn_function(); \
} while (0)

thanks,
StefanK
> 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
>>
>>>
>>>
>>
>