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

[Stefan Karlsson]

On 2014-11-14 03:39, David Holmes wrote:
> On 14/11/2014 3:24 AM, 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.
>>
>> 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.
>
> This was my concern too - what actually gets returned in those cases! 
> But if this has been verified, and as you say we don't see problems 
> because of this, then the change to just THREAD seems okay.
>
> Personally though I prefer the alternative style:
>
>  Klass* ConstantPool::klass_ref_at(int which, TRAPS) {
>   Klass* k = klass_at(klass_ref_index_at(which), CHECK_NULL);
>   return k;
>  }
>
> I'll leave it up to Stefan. :)

OK. I'll push the patch as it's currently written. Then if we decide to 
use temporary variables instead, we can do that as a separate patch.

Thanks,
StefanK

>
> Cheers,
> David
>
>
>> 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
>>>>
>>>>
>>>