RFR(XS): 8145333: -XX:+EnableJVMCI -XX:+UseJVMCICompiler -XX:-EnableJVMCI makes JVM crash
Christian Thalinger
christian.thalinger at oracle.com
Mon Feb 22 19:30:39 UTC 2016
> On Feb 20, 2016, at 8:12 AM, Jamsheed C m <jamsheed.c.m at oracle.com> wrote:
>
> Hi Chris,
>
> I missed handling a valid case where UnlockExperimentalVMOptions/UnlockDiagnosticVMOptions are made false after experimental/diagnostic jvmci flags are modified.
>
> i.e : bin/java -XX:+UnlockExperimentalVMOptions -XX:-EnableJVMCI -XX:+UseJVMCICompiler -XX:-UnlockExperimentalVMOptions -version
>
> made changes to handle this case.
>
> code change
> + // Check consistency of diagnostic flags if UnlockDiagnosticVMOptions is true
> + // or not default. UnlockDiagnosticVMOptions is default true in debug builds.
> + if (UnlockDiagnosticVMOptions ||
> + !FLAG_IS_DEFAULT(UnlockDiagnosticVMOptions)) {
>
>
> + // Check consistency of experimental flags if UnlockExperimentalVMOptions is
> + // true or not default.
> + if (UnlockExperimentalVMOptions ||
> + !FLAG_IS_DEFAULT(UnlockExperimentalVMOptions)) {
Oh, good. But please don’t break the line. No new webrev needed.
>
>
> revised webrev: http://cr.openjdk.java.net/~jcm/8145333/webrev.06/ <http://cr.openjdk.java.net/~jcm/8145333/webrev.06/>
>
> Thanks and Best Regards,
> Jamsheed
>
> On 2/20/2016 5:10 AM, Jamsheed C m wrote:
>> Thanks, Chris.
>>
>> Best Regards,
>> Jamsheed
>>
>> On 2/19/2016 10:56 PM, Christian Thalinger wrote:
>>> Looks good. Thanks.
>>>
>>>> On Feb 19, 2016, at 5:38 AM, Jamsheed C m <jamsheed.c.m at oracle.com <mailto:jamsheed.c.m at oracle.com>> wrote:
>>>>
>>>> Hi Chris,
>>>>
>>>> Made all suggested changes.
>>>>
>>>> Revised webrev: http://cr.openjdk.java.net/~jcm/8145333/webrev.05/ <http://cr.openjdk.java.net/%7Ejcm/8145333/webrev.05/>
>>>>
>>>> Thanks and Best Regards,
>>>> Jamsheed
>>>>
>>>> On 2/19/2016 10:53 AM, Jamsheed C m wrote:
>>>>>
>>>>>
>>>>> On 2/18/2016 11:21 PM, Christian Thalinger wrote:
>>>>>> + if (EnableJVMCI)
>>>>>> + return true;
>>>>>>
>>>>>> Please use { } for all if statements.
>>>>> Ok.
>>>>>>
>>>>>> Thanks for the comments; it’s easier to understand now. May I suggest to do:
>>>>>>
>>>>>> int skip_fail_count;
>>>>>> if (!FLAG_IS_DEFAULT(EnableJVMCI)) {
>>>>>> skip_fail_count = 1;
>>>>>> } else {
>>>>>> skip_fail_count = 0;
>>>>>> }
>>>>> Ok.
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>>>>
>>>>>>> On Feb 18, 2016, at 6:26 AM, Jamsheed C m <jamsheed.c.m at oracle.com <mailto:jamsheed.c.m at oracle.com>> wrote:
>>>>>>>
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> Made all suggested changes.
>>>>>>>
>>>>>>> Revised webrev: <http://cr.openjdk.java.net/%7Ethartmann/8145333/webrev.04/>http://cr.openjdk.java.net/~thartmann/8145333/webrev.04/ <http://cr.openjdk.java.net/~thartmann/8145333/webrev.04/>
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jamsheed
>>>>>>>
>>>>>>> On 2/18/2016 6:24 AM, Jamsheed C m wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/18/2016 12:36 AM, Christian Thalinger wrote:
>>>>>>>>>
>>>>>>>>>> On Feb 17, 2016, at 4:52 AM, Jamsheed C m < <mailto:jamsheed.c.m at oracle.com>jamsheed.c.m at oracle.com <mailto:jamsheed.c.m at oracle.com>> wrote:
>>>>>>>>>>
>>>>>>>>>> experimental and diagnostic flags are under condition checks now.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~thartmann/8145333/webrev.03/ <http://cr.openjdk.java.net/%7Ethartmann/8145333/webrev.03/>
>>>>>>>>>
>>>>>>>>> I’m not very happy about the new macros but I can see why it’s useful.
>>>>>>>>>
>>>>>>>>> +#if INCLUDE_JVMCI
>>>>>>>>> +
>>>>>>>>> +// Check consistency of jvmci vm argument settings.
>>>>>>>>> +bool Arguments::check_jvmci_args_consistency() {
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> + if (!EnableJVMCI && is_any_jvmci_arg_values_changed()) {
>>>>>>>>> + print_jvmci_arg_inconsistency_error_message();
>>>>>>>>> + return false;
>>>>>>>>>
>>>>>>>>> Please remove all these empty lines. There are others as well.
>>>>>>>>>
>>>>>>>>> +bool is_any_jvmci_arg_values_changed() {
>>>>>>>>>
>>>>>>>>> This method needs another name.
>>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>> + int check_count = 0, fail_count = 0;
>>>>>>>>>
>>>>>>>>> I don’t like the check_count/fail_count logic because it’s fragile.
>>>>>>>> As EanbleJVMCI flag cannot be avoided from macro expansion logic, i had to use some technique to exclude this flag from failure decision making.
>>>>>>>> Check_count/fail_count logic was added as a solution for this. i.e When EnableJVMCI is changed, code requires at-least 2 check failures to decide a real failure. otherwise one failure is sufficient.
>>>>>>>>
>>>>>>>> if i remove this logic, i will have to use strcmp(#FLAG, "EnableJVMCI") which will add a little more instructions that will execute at-least once for every flag consistent run.
>>>>>>>>
>>>>>>>> let me know if i need to take second approach. or if existing code is ok.
>>>>>>>>>
>>>>>>>>> +// Check if any jvmci global defaults changed.
>>>>>>>>> +bool is_any_jvmci_arg_values_changed();
>>>>>>>>> +// Print jvmci args inconsistency error message.
>>>>>>>>> +void print_jvmci_arg_inconsistency_error_message();
>>>>>>>>>
>>>>>>>>> These should be in some kind of namespace or class. Maybe put them into: class JVMCIGlobals
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Jamsheed
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Best Regards,
>>>>>>>>>> Jamsheed
>>>>>>>>>>
>>>>>>>>>> On 2/16/2016 6:50 PM, Jamsheed C m wrote:
>>>>>>>>>>> Hi Chris,
>>>>>>>>>>>
>>>>>>>>>>> Revised webrev: <http://cr.openjdk.java.net/%7Ethartmann/8145333/webrev.02/>http://cr.openjdk.java.net/~thartmann/8145333/webrev.02/ <http://cr.openjdk.java.net/~thartmann/8145333/webrev.02/>
>>>>>>>>>>>
>>>>>>>>>>> This code take care of every jvmci flags automatically.
>>>>>>>>>>>
>>>>>>>>>>> Best Regards,
>>>>>>>>>>> Jamsheed
>>>>>>>>>>>
>>>>>>>>>>> On 2/13/2016 3:24 AM, Christian Thalinger wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> On Feb 12, 2016, at 7:12 AM, Jamsheed C m < <mailto:jamsheed.c.m at oracle.com>jamsheed.c.m at oracle.com <mailto:jamsheed.c.m at oracle.com>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Chris,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2/12/2016 10:12 PM, Christian Thalinger wrote:
>>>>>>>>>>>>>> Well, now you have to manually check all flags that had a constraint directive. That is the annoying part and what I complained about in:
>>>>>>>>>>>>> check against original value, and if not fail ?
>>>>>>>>>>>>
>>>>>>>>>>>> Same check for all flags; they all depend on +EnableJVMCI.
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8145357>https://bugs.openjdk.java.net/browse/JDK-8145357 <https://bugs.openjdk.java.net/browse/JDK-8145357>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also, commandLineFlagConstraintsJVMCI.{cpp,hpp} files are now empty and I think we should remove them.
>>>>>>>>>>>>> i kept the file as there can be future use. Ok, i will remove it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Best Regards,
>>>>>>>>>>>>> Jamsheed
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Feb 11, 2016, at 9:47 PM, Jamsheed C m < <mailto:jamsheed.c.m at oracle.com>jamsheed.c.m at oracle.com <mailto:jamsheed.c.m at oracle.com>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Chris,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> revised webrev: <http://cr.openjdk.java.net/%7Ethartmann/8145333/webrev.01/>http://cr.openjdk.java.net/~thartmann/8145333/webrev.01/ <http://cr.openjdk.java.net/~thartmann/8145333/webrev.01/>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Best Regards,
>>>>>>>>>>>>>>> Jamsheed
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2/11/2016 11:31 PM, Christian Thalinger wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Feb 11, 2016, at 3:47 AM, Jamsheed C m < <mailto:jamsheed.c.m at oracle.com>jamsheed.c.m at oracle.com <mailto:jamsheed.c.m at oracle.com>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Request for review
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> bug url: <https://bugs.openjdk.java.net/browse/JDK-8145333>https://bugs.openjdk.java.net/browse/JDK-8145333 <https://bugs.openjdk.java.net/browse/JDK-8145333>
>>>>>>>>>>>>>>>>> web url: <http://cr.openjdk.java.net/%7Ethartmann/8145333/webrev.00/>http://cr.openjdk.java.net/~thartmann/8145333/webrev.00/ <http://cr.openjdk.java.net/~thartmann/8145333/webrev.00/>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> That looks alright but we should remove the constraints from all the JVMCI command line flags because the way we use them is not supported.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Also, can you change the error message? Right now it looks like this:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> $ ./build/macosx-x86_64-normal-server-release/jdk/bin/java -XX:+UnlockExperimentalVMOptions -XX:+UseJVMCICompiler
>>>>>>>>>>>>>>>> EnableJVMCI must be enabled
>>>>>>>>>>>>>>>> Improperly specified VM option 'UseJVMCICompiler'
>>>>>>>>>>>>>>>> Error: Could not create the Java Virtual Machine.
>>>>>>>>>>>>>>>> Error: A fatal exception has occurred. Program will exit.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I think with your changes we will only see the first line without mentioning UseJVMCICompiler. Is that correct?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Fix Summary: added jvmci flag consistency check after arg parsing.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> tests: jprt
>>>>>>>>>>>>>>>>> unit test: command line verification
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Best Regards,
>>>>>>>>>>>>>>>>> Jamsheed
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160222/31e984fb/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list