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

[Stefan Karlsson]

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

1) report_should_not_reach_here is used by assert and guarantee and we 
have infrastructure to ignore/skip those by specifying a filename and a 
line number.
2) We would never reach the BREAKPOINT code.

>
> In any case, hidden in ShouldNotReachHere is good.

Great.

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