RFR(S): 8239915: Zero VM crashes when handling dynamic constant

[David Holmes]

Hi Felix,

On 27/02/2020 12:40 pm, Yangfei (Felix) wrote:
> New webrev: http://cr.openjdk.java.net/~fyang/8239915/webrev.01
> 
> Does that look better?

Test update looks good.

I'm not clear on the index versus cache_index issue, but see now how it 
leads to the failure. Calling InterpreterRuntime::resolve_ldc to always 
resolve is fine.

Thanks,
David

> Thanks,
> Felix
> 
>>
>> Hi David,
>>
>>    Thanks for reviewing this.
>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Wednesday, February 26, 2020 1:59 PM
>>> To: Yangfei (Felix) <felix.yang at huawei.com>;
>>> hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR(S): 8239915: Zero VM crashes when handling dynamic
>>> constant
>>>
>>> Hi Felix,
>>>
>>> On 26/02/2020 12:18 pm, Yangfei (Felix) wrote:
>>>> Hi,
>>>>
>>>> Please help review this patch fixing a zero vm crash issue.
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8239915
>>>> Webrev: http://cr.openjdk.java.net/~fyang/8239915/webrev.00/
>>>>
>>>> The dynamic constant is not resolved at [1] and this leads to the crash.
>>>> Patch fixes the issue by always resolving the dynamic constant at the site.
>>>> This aligns with the template interpreter (TemplateTable::condy_helper).
>>>
>>> Not familiar with this code but I don't understand the failure mode.
>>> If we haven't cached the resolution result we will get NULL and
>>> re-resolve it. If we have cached it then it should be correct. Either
>>> way no crash, and I certainly don't see how we end up at:
>>>
>>> 2406           default:  ShouldNotReachHere();
>>
>> Using cp_index here causes an out-of-bound array access.  As a result, 'result'
>> is not NULL here but an invalid value.
>> In case of JVM_CONSTANT_Dynamic, InterpreterRuntime::resolve_ldc()
>> got/set the resolved reference into/from the cache with a cache_index instead
>> of cp_index.
>> See ConstantPool::resolve_constant_at_impl for details.
>>
>>>
>>>> Tier1 tested with x86-64 zero release build.
>>>> Newly added test case fails without this fix and passes with this fix.
>>>
>>> Note that for the tests the source directory structure does not
>>> generally indicate a package structure, so no need to put the test
>>> classes into a package unless it is actually required for the test to
>>> pass. (And even then no need for the package used to map to the test
>>> directory structure.)
>>
>> Good suggestion.  I will remove the package specifier from the test case and
>> propose a new webrev.
>>
>> Best regards,
>> Felix