RFR: 8064811: Use THREAD instead of CHECK_NULL in return statements

[Volker Simonis]

Hi Stefan,

thanks a LOT for doing this change. This one was on my list since
years (see "6889002: CHECK macros in return constructs lead to
unreachable code" at https://bugs.openjdk.java.net/browse/JDK-6889002)
but I never finalized it.

Your change looks good and I think it must be stressed that your code
doesn't change any functionality because it only eliminates dead code!

If we will have code after your change which fails to check for
pending exceptions or which expects to get a certain result in the
case of a pending exceptions it was just es wrong already before your
change because the exception check was never reached. So unless we see
one of these errors I don't think it will be necessary to introduce
all these temporary variables.

When I started to work on this problem years ago I went the other way
round: I looked at the called functions (i.e. klass_at() in your
example) to check if they already return NULL in the case of a pending
exception. As far as I remember they all behaved "well" - otherwise we
would already have seen some problems with the current implementation.

Thanks and best regards,
Volker

PS: our HPUX C++ compiler will love this change:)


On Thu, Nov 13, 2014 at 4:28 PM, Stefan Karlsson
<stefan.karlsson at oracle.com> wrote:
>
> On 2014-11-13 16:33, Coleen Phillimore wrote:
>>
>>
>> The thing that I worry about with this change is that if someone adds code
>> later after the return, they'll miss changing the THREAD parameter back into
>> CHECK.
>
>
>
> I was thinking the same, but I knew the we used this idiom in other places
> in the JVM and thought that changing these would be OK. An alternative
> approach would be to always read out the value into a variable:
>
>  Klass* ConstantPool::klass_ref_at(int which, TRAPS) {
>   Klass* k = klass_at(klass_ref_index_at(which), CHECK_NULL);
>   return k;
>  }
>
> I can do that if people feel more comfortable with it.
>
> Thanks,
> Stefank
>
>
>>   But maybe it's okay because the thing returned will be NULL and code is
>> likely to crash on someone trying to use the value returned. Ok.  This is a
>> good cleanup.  I'm surprised there weren't more.
>>
>> Thanks,
>> Coleen
>>
>> On 11/13/14, 9:59 AM, Stefan Karlsson wrote:
>>>
>>> Hi all,
>>>
>>> Please, review this patch to replace usages of the CHECK_ macros in
>>> return statements, with the THREAD define.
>>>
>>> http://cr.openjdk.java.net/~stefank/8064811/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8064811
>>>
>>> From the bug report:
>>>
>>> Take the following method as an example:
>>>  Klass* ConstantPool::klass_ref_at(int which, TRAPS) {
>>>   return klass_at(klass_ref_index_at(which), CHECK_NULL);
>>>  }
>>>
>>> This will expand into:
>>>  Klass* ConstantPool::klass_ref_at(int which, TRAPS) {
>>>   return klass_at(klass_ref_index_at(which), THREAD);
>>>   if (HAS_PENDING_EXCEPTIONS) {
>>>     return NULL;
>>>   }
>>>   (void)(0);
>>>  }
>>>
>>> The if-statement will never be reached.
>>>
>>> We have seen cases where the compiler warns about this, and the recent
>>> change to enable -Wreturn-type will make this more likely to happen.
>>>
>>> The suggested solution is to change the example above into:
>>>  Klass* ConstantPool::klass_ref_at(int which, TRAPS) {
>>>   return klass_at(klass_ref_index_at(which), THREAD);
>>>  }
>>>
>>> thanks,
>>> StefanK
>>
>>
>