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

[Yangfei (Felix)]

New webrev: http://cr.openjdk.java.net/~fyang/8239915/webrev.01  

Does that look better?  

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