RFR 8030011: Update Hotspot version string output

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 4/22/14 7:32 PM, Alejandro E Murillo wrote:
>
> On 4/22/2014 6:12 PM, Vladimir Kozlov wrote:
>> 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"?
> sounds good!
>>
>> 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.
> it's gone!
>>
>>>>
>>>> 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);
>>  }
>>
> you are right. I recall having added that earlier :(
> it's back int the webrev
>>>
>>>>
>>>> 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 see. I don't think it's needed
>
> I think I incorporated all the changes David, John and you suggested and
> started some sanity testing;
> Here's is the latest webrev:
>
> http://cr.openjdk.java.net/~amurillo/9/8030011/
>
> Please review (don't forget to refresh each file on your browser) and
> let me know if I missed anything.
> thanks guys!
>