RFR 8030011: Update Hotspot version string output

[Vladimir Kozlov]

On 4/22/14 4:42 PM, Alejandro E Murillo wrote:
>
> On 4/21/2014 1:18 PM, Vladimir Kozlov wrote:
>> Hi Alejandro,
>>
>> I don't think we need to rename make/hotspot_version file. It is still
>> used to set JVM's version string and not JDK's version.
>
>>
>> Next assert message is not consistent with previous messages which use
>> "vm", I think it should be "vm" here too:
>>
>>   DEBUG_ONLY(assert_digits(vm_build_num, offset, "wrong JDK build
>> number"));
> we do not have hotspot build number anymore, so I think we should not
> mention hotspot build numberls

Can we simple say "wrong build number"?

So you don't want update build number in make/*_version files? ;)
Yes, I see in your example of JPRT build VM does not have build number 
anymore.

>>
>> Abstract_VM_Version::jvm_version() should include micro version. See
>> JVM_GetVersionInfo() in jvm.cpp and jvm_version_info in
>> jdk/src/share/javavm/export/jvm.h.
> I added that here, is that what you are referring?
> http://cr.openjdk.java.net/~amurillo/9/8030011/src/share/vm/runtime/vmStructs.cpp.udiff.html
>
> http://cr.openjdk.java.net/~amurillo/9/8030011/src/share/vm/runtime/vm_version.hpp.udiff.html

No, I mean next code should encode micro version too:

  unsigned int Abstract_VM_Version::jvm_version() {
    return ((Abstract_VM_Version::vm_major_version() & 0xFF) << 24) |
           ((Abstract_VM_Version::vm_minor_version() & 0xFF) << 16) |
+         ((Abstract_VM_Version::vm_micro_version() & 0xFF) <<  8) |
           (Abstract_VM_Version::vm_build_number() & 0xFF);
  }

>
>>
>> Use corresponding test in jdk for testing of these changes:
>>
>> jdk/test/sun/misc/Version/Version.java
> yeah, I run that test as part of jprt full builds,
> That test handles both JDK and Hotspot express versions

Good.

>>
>> jvm.h: Next comment is not accurate:
>>
>> +    /* VM version string: JDK version string     */
>>
>> If we build VM separately (for example, in JPRT) VM version will not
>> be JDK version in which VM is installed. It will take numbers either
>> from passed make parameters or from make/hotspot_version. I think it
>> should say:
>>
>> +    /* VM version string follows the JDK release version naming
>> convention
>> +     * <major>.<minor>.<micro>-bxx[-<identifier>][-<debug_flavor>]
>>
>> Based on your examples [-<identifier>][-<debug_flavor>] is still used
>> so it should be reflected in the comment.
> Let me make that explicit.
>>
>> Don't remove next comments from vm_version.cpp but fix it ("follow the
>> JDK release"):
>>
>> -// HOTSPOT_RELEASE_VERSION must follow the release version naming
>> convention
>> -// <major_ver>.<minor_ver>-b<nn>[-<identifier>][-<debug_target>]
> ok
>>
>> You did not show default VM version example when VM is built locally
>> by engineer.
> It will be similar to the hotspot only jprt build. There will be a
> mismatch,
> I tested by dropping the resulting libjvm  into a promoted build, so it
> looks like this:
>
> java version "1.9.0-ea"
> Java(TM) SE Runtime Environment (build 1.9.0-ea-b01)
> Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-internal-debug, mixed mode)

okay

>
>>
>> Please test that correct version string is constructed when you build
>> VM using make/build.sh, for example 'sh make/build.sh debug LP64=1'
> Haven't tested that, thanks for pointing that out.

thank you

>>
>> Next comment change in buildtree.make is not correct because
>> HOTSPOT_RELEASE_VERSION make parameter does not include
>> HOTSPOT_BUILD_VERSION:
>>
>> -# HOTSPOT_RELEASE_VERSION - <major>.<minor>-b<nn> (11.0-b07)
>> +# HOTSPOT_RELEASE_VERSION - JRE_RELEASE_VERSION plus
>> HOTSPOT_BUILD_VERSION
>>
>> see JPRT logs where HOTSPOT_BUILD_VERSION is set separately.
> let me check that again
>>
>> I think next change in make/defs.make is not safe (removing make
>> parameter) due to complexity of our builds:
>>
>> -MAKE_ARGS += HOTSPOT_RELEASE_VERSION=$(HOTSPOT_RELEASE_VERSION)
> I checked the code, and HOTSPOT_RELEASE_VERSION is only used in
> vm_version.cpp,
> so I think is fine to remove it. Note that if we keep it there, since
> the JDK version string
> sometimes might have time stamps, it may affect ccache,  that's why at
> some point they
> separated the options for vm_version.cpp from the options for other
> components. See this
> comment on vm,make:
>
> # This is VERY important! The version define must only be supplied to
> vm_version.o
> # If not, ccache will not re-use the cache at all, since the version
> string might contain
> # a time and date.
>

I was concern that it will not be passed into nested make so that, for 
example, buildtree.make will not get it.

>
>>
>>
>> I know that windows build is mess. Please verify it carefully. For
>> example, you changed names JDK_*_VER to JDK_*_VERSION in def.make but
>> build.make uses them:
>>
>> JDK_VER=$(JDK_MINOR_VER),$(JDK_MICRO_VER),$(JDK_UPDATE_VER),$(JDK_BUILD_NUMBER)
>>
> That was a typo. Note that I changed the left handside, so they were
> incorrectly reassigning those.
> The places were JDK_MINOR_VER is used, the value is read from
> jdk_version (formerly hotspot_version)

Okay.

Thanks,
Vladimir

>
> Thanks a lot for the feedback!
> Alejandro
>>
>>
>> Regards,
>> Vladimir
>>
>> On 4/21/14 10:13 AM, Alejandro E Murillo wrote:
>>>
>>> On 4/18/2014 6:50 PM, John Coomes wrote:
>>>> Alejandro E Murillo (alejandro.murillo at oracle.com) wrote:
>>>>> Please review this change to make the hotspot related output
>>>>> produced by
>>>>> "java -version"
>>>>> match the corresponding JDK output:
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~amurillo/9/8030011/
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8030011
>>>>>
>>>>> Note that we initially wanted to obtain more information from the repo
>>>>> being built and add
>>>>> it to the hotspot version output, but that will require changes in the
>>>>> JPRT, so
>>>>> we have decided to split the change in 2 bugs. One to make the
>>>>> version match
>>>>> (above webrev) and another one, an RFE, to add additional information
>>>>> extracted from the repo.
>>>> Generally looks good, but I agree with David that the long lines
>>>> should be wrapped, and it might even be clearer to separate them into
>>>> two variables, e.g.,
>>>>
>>>> JDK_VERSION_DEFS = -DJDK_MAJOR_VERSION="\"$(JDK_MAJOR_VERSION)\"" \
>>>>            -DJDK_MINOR_VERSION="\"$(JDK_MINOR_VERSION)\"" \
>>>>            ... other defs ...
>>>> VERSION_DEFS  = -DHOTSPOT_RELEASE_VERSION="\"$(HS_BUILD_VER)\"" \
>>>>         -DJRE_RELEASE_VERSION="\"$(JRE_RELEASE_VER)\"" \
>>>>         $(JDK_VERSION_DEFS)
>>>>
>>>> Also the change to this part of vm.make differs between at least the
>>>> solaris version and the linux version (didn't check bsd or windows).
>>>> Please make them all consistent.
>>>>
>>>>> Note that in the current version of vm_version.cpp, there is no error
>>>>> checking in product mode,
>>>>> I was suggested to make that explicit.
>>>> I like the additional error checking.  I think you could eliminate the
>>>> 4 copies of similar code with a function that does the checks and sets
>>>> the field, e.g.,
>>>>
>>>>      void set_version_field(int * version_field, const char *
>>>> version_str,
>>>>                const char * const assert_msg) {
>>>>         if (version_str != NULL ...) {
>>>>      DEBUG_ONLY(assert_digits(version_str, assert_msg));
>>>>      *version_field = atoi(version_str);
>>>>         }
>>>>      }
>>>>
>>>> -John
>>> Thanks John,
>>> working on adding these changes and sanity testing
>>>
>>> Thanks
>>>
>