RFR: 8245717: VM option "-XX:EnableJVMCIProduct" could not be repetitively enabled

[Xiaohong Gong]

Hi Vladimir,

> Looks good to me.

Thanks so much for your reviewing! 

Best Regards,
Xiaohong Gong

-----Original Message-----
From: Vladimir Kozlov <vladimir.kozlov at oracle.com> 
Sent: Friday, May 29, 2020 12:45 AM
To: Xiaohong Gong <Xiaohong.Gong at arm.com>; David Holmes <david.holmes at oracle.com>; hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Cc: nd <nd at arm.com>
Subject: Re: RFR: 8245717: VM option "-XX:EnableJVMCIProduct" could not be repetitively enabled

Looks good to me.

Thanks,
Vladimir K

On 5/27/20 1:02 AM, Xiaohong Gong wrote:
> Hi David,
> 
>   > On 26/05/2020 4:57 pm, Xiaohong Gong wrote:
>   > > Hi,
>   > >
>   > > Could you please help to review this simple patch? It fixes the
>   > issue
>   > > that JVM crashes in debug mode when the vm option
>   > "-XX:EnableJVMCIProduct" is enabled repetitively.
>   > >
>   > > JBS: https://bugs.openjdk.java.net/browse/JDK-8245717
>   > > Webrev: http://cr.openjdk.java.net/~xgong/rfr/8245717/webrev.00/
>   > >
>   > > Repetitively enabling the vm option "-XX:+EnableJVMCIProduct" in
>   > the
>   > > command line makes the assertion fail in debug mode:
>   > "assert(is_experimental(), sanity)".
>   > >
>   > > It happens when the VM iteratively parses the options from
>   > command
>   > > line. When the matched option is "-XX:+EnableJVMCIProduct", the
>   > > original experimental JVMCI flags will be converted to product
>   > mode,
>   > > with the above assertion before it. So if all the JVMCI flags
>   > have
>   > > been converted to the product mode at the first time parsing
>   > "-XX:+EnableJVMCIProduct", the assertion will fail at the second
>   > time it is parsed.
>   > >
>   > > A simple fix is to just ignoring the conversion if this option
>   > has been parsed.
>   >
>   > Seems a reasonable approach given the already complex handling of
>   > this flag.
>   >
>   > > Testing:
>   > > Tested jtreg
>   > > hotspot::hotspot_all_no_apps,jdk::jdk_core,langtools::tier1
>   > > and jcstress:tests-custom, and all tests pass without new
>   > failure.
>   >
>   > I think adding a regression test in
>   > ./compiler/jvmci/TestEnableJVMCIProduct.java would be appropriate.
> 
> Thanks for your review and it’s a good idea to add the regression test.
> I have added the test in the new patch:
> http://cr.openjdk.java.net/~xgong/rfr/8245717/webrev.01/ .
> 
> Could you please take a look at it? Thank you!
> 
> Thanks,
> Xiaohong Gong
>