RFR(XS): 8145333: -XX:+EnableJVMCI -XX:+UseJVMCICompiler -XX:-EnableJVMCI makes JVM crash

[Jamsheed C m]

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/~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 
>>>>> <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/~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 
>>>>>>>> <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
>>>>>>>>>
>>>>>>>>> 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 
>>>>>>>>>> <jamsheed.c.m at oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Chris,
>>>>>>>>>>
>>>>>>>>>> revised webrev: 
>>>>>>>>>> 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 
>>>>>>>>>>>> <jamsheed.c.m at oracle.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Request for review
>>>>>>>>>>>>
>>>>>>>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8145333
>>>>>>>>>>>> web url: 
>>>>>>>>>>>> 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/20160219/7aed1111/attachment-0001.html>