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

[Coleen Phillimore]

Question below...  not to belabor the point.

On 11/5/14, 3:45 PM, Stefan Karlsson wrote:
> 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)
>
Rather than have all this, why not have report_should_not_reach_here() 
have the NORETURN_ATTRIBUTE.

In any case, hidden in ShouldNotReachHere is good.

Coleen

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