RFR (S): 8230395: Code checks for NULL value returned from NEW_C_HEAP_ARRAY which can not happen

[David Holmes]

Hi Leo,

Thanks for looking at this.

On 19/09/2019 9:31 pm, Leo Korinth wrote:
> On 19/09/2019 07:57, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8230395
>> webrev: http://cr.openjdk.java.net/~dholmes/8230395/webrev/
>>
>> First, thanks to Leo for going through and finding the places that 
>> needed to be fixed. I didn't go looking for others.
> 
> I tried to find all places for NEW_C_HEAP_ARRAY. Thanks for not only 
> fixing the problematic use but also doing some cleanup around it! It was 
> not obvious for me where to remove the check and where to add 
> RETURN_NULL, so thank you for doing this.

Noticing that we were new'ing CHeapObj made it clearer in the perf code 
that there was no point using RETURN_NULL, as we would abort on the new 
anyway. :) Otherwise it's all pretty subjective.

Cheers,
David

>>
>> For the most part I have deleted the NULL checks. A few notes:
>>
>> - changed to NEW_C_HEAP_ARRAY_RETURN_NULL in a couple of places where 
>> it was more consistent with the general recoverability approach of the 
>> code in question (and did the opposite change in one place)
>> - for the "perf" changes I also noticed that we were checking for NULL 
>> after doing "new SomeCHeapObj()" which also cannot return NULL, so 
>> removed those NULL checks too
>> - the JVMCI thread counter changes became a bit more extensive due to 
>> the fact a boolean method could now only every return true. So it was 
>> changed to void and that had to filter all the way back up to the 
>> JVMCI Java API. The JVMCI/Graal folk need to approve this
>>
>> Testing: tier 1-3 sanity tests (in progress)
>>
>> Thanks,
>> David
> 
> This change looks good to me (I am not a reviewer).
> 
> Thanks,
> Leo