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

[Stefan Karlsson]

Hi Volker,

On 2014-11-13 18:24, Volker Simonis wrote:
> 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.

I didn't CHECK for already open bugs/enhancements for this ;)

>
> 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 for verifying this.
>
> Thanks and best regards,
> Volker
>
> PS: our HPUX C++ compiler will love this change:)

Great!

Thanks,
StefanK

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