RFR(S): 8203329: JDWP/JDI VM information string is incorrect

[Chris Plummer]

Ok, I'll update the comment. I think I'll still mention and CDS and 
maybe add AOT as examples.

thanks,

Chris

On 6/6/18 7:01 PM, David Holmes wrote:
> Hi Chris,
>
> Great sleuthing on this one and thanks for explaining things for me in 
> the bug report!
>
> The fix you have addresses the problem observed with the "vm info" 
> string. One small request, can you update this comment in thread.cpp 
> please:
>
>  // We need this for ClassDataSharing - the initial vm.info property 
> is set
>   // with the default value of CDS "sharing" which may be reset through
>   // command line options.
>   reset_vm_info_property(CHECK_JNI_ERR);
>
> As we actually need that for any piece of information that might be 
> used in the vm info string initially, that can subsequently change due 
> to option processing e.g. UseSharedSpaces and UseAOT. Suggestion:
>
> // We need this to update the vm.info property in case any flags used
> // to initially define it have been changed with command-line options.
> reset_vm_info_property(CHECK_JNI_ERR);
>
> Thanks!
>
> ---
>
> That said I think we can simplify things somewhat now you discovered 
> the bug in reset_vm_info_property. If we simply update the native 
> vm_info property as soon as we've completed argument parsing, then we 
> will have the right value in place for copying to the Java level 
> during java.lang.System initialization. We don't need any of the 
> existing code in reset_vm_info because we copy the correct vm_info in 
> the first place. In short you should be able to fix this with simply:
>
>   jint parse_result = Arguments::parse(args);
>   if (parse_result != JNI_OK) return parse_result;
>
>   os::init_before_ergo();
>
>   jint ergo_result = Arguments::apply_ergo();
>   if (ergo_result != JNI_OK) return ergo_result;
>
> + const char *vm_info = VM_Version::vm_info_string();
> + Arguments::PropertyList_update_value(Arguments::system_properties(), 
> "java.vm.info", vm_info);
> ...
> -  // We need this for ClassDataSharing - the initial vm.info property 
> is set
> -  // with the default value of CDS "sharing" which may be reset through
> -  // command line options.
> -  reset_vm_info_property(CHECK_JNI_ERR);
>
> and delete reset_vm_info_property.
>
> Just a thought - feel free to defer - it's really a runtime issue 
> anyway. :)
>
> Thanks,
> David
>
> On 7/06/2018 8:24 AM, Chris Plummer wrote:
>> Hello,
>>
>> Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203329
>> http://cr.openjdk.java.net/~cjplummer/8203329/webrev.00/
>>
>> The native version of the java.vm.info property was getting out of 
>> sync with the java version. Details can be found here:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8203329?focusedCommentId=14185679&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14185679 
>>
>>
>> thanks,
>>
>> Chris