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

[Yangfei (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