RFR 8030011: Update Hotspot version string output

[Alejandro E Murillo]

Thanks Vladimir
Alejandro

On 4/23/2014 11:53 AM, Vladimir Kozlov wrote:
> 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!
>>

-- 
Alejandro