[verona.stage] RFR 8087203: Add support for PATCH field and remove unused fields of new version string

[Alejandro E Murillo]

Hi David,
thanks for the review, see below

On 6/18/2015 1:40 AM, David Holmes wrote:
> Hi Alejandro,
>
> I looked at the hotspot and JDK changes.
>
> On 17/06/2015 8:55 AM, Alejandro E Murillo wrote:
>>
>> Please review these changes:
>>
>> Bug:  https://bugs.openjdk.java.net/browse/JDK-8087202
>> Webrev: http://cr.openjdk.java.net/~amurillo/9/8087202
>
> hotspot/make/Makefile
>
> + #  VERSION_PATCH Security number for version (e.g. 0)
>
> Comment is wrong.
cut and paste. Will fix
>
> ---
>
> hotspot/src/share/vm/prims/jvm.cpp
>
> 3659   info->patch_version = 0;
>
> Why is this hard-wired to zero? Surely this should be a build variable.
good catch! That should be initialized from VM_Version as well
(was mimicking update version, but that was always zero for hotspot
which is not the case for patch). New webrev:
http://cr.openjdk.java.net/~amurillo/9/8087202.v2/hotspot/src/share/vm/prims/jvm.cpp.udiff.html
>
> ---
>
> hotspot/src/share/vm/prims/jvm.h
> jdk/src/java.base/share/native/include/jvm.h
>
> These two files should be identical, but now have different comments 
> in places.
will add the missing comments
>
> 1188     unsigned int patch_version : 8;
> 1215     unsigned int patch_version : 8;
>
> In each case you removed two 8 bit fields and added one 8 bit field so 
> your bitfields no longer add up to 32 (8+8+16). But these structs seem 
> obsolete (due to no hotspot-express) even before the new version 
> changes. So I think there is still a bit of follow up work to do here.
oh yes,  looks like I need to keep the size of the whole structure,
to avoid any misalignment that may  impact performance.
I thought about declaring patch to be short (16) instead,
but I ended up adding a padding member, reserved3, size 8,
to keep the structure size unchanged. I guess I could have changed
reserved2 to 24, let me know if there's any advantage using either of those.
New webrev:
http://cr.openjdk.java.net/~amurillo/9/8087202.v2/hotspot/src/share/vm/prims/jvm.h.udiff.html

>
> ---
>
> hotspot/src/share/vm/runtime/java.hpp
>
> 110                   _partially_initialized(false),
>
> Isn't the whole partially vs fully initialized issue dead now? Else 
> why delete the other code pertaining to that?
yeah, I should have removed the remaining uses of that.
It's gone now.
>
> ---
>
> jdk/src/java.base/share/classes/sun/misc/Version.java.template
>
>   * @since JDK9
>
> Is that the right format? I would have expected @since 9.
I meant to double check that. I changed it
>
>  248     private static native boolean getJvmVersionInfo();
>
> Does that method still need to return a boolean ? See next comment
>
> ---
>
> jdk/src/java.base/share/native/libjava/Version.c
>
>   50     if (!JDK_InitJvmHandle()) {
>   51         JNU_ThrowInternalError(env, "Handle for JVM not found for 
> symbol lookup");
>   52         return JNI_FALSE;
>   53     }
>   54     func_p = (GetJvmVersionInfo_fp) 
> JDK_FindJvmEntry("JVM_GetVersionInfo");
>   55     if (func_p == NULL) { --
>   56         return JNI_FALSE;
>   57     }
>
> this seems dead code now and so the method can't fail or throw an 
> exception.
I thought about changing that, but I prefer if it's done
as a separate change for now. will file a follow up bug,
Are you ok with that?

New webrev here (I tested these with JPRT, testsets hotspot and pit):

http://cr.openjdk.java.net/~amurillo/9/8087202.v2/

Thanks
Alejandro

>
> Thanks,
> David
> -----
>
>> These are intended to:
>>
>> (1)  Add support for the patch field of the new version string format
>> (2) Remove unused fields remaining from the old version string format
>> (3) Patch some jaxp initialization code to be able to handle the new
>> version string.
>>        this will be pushed under a different bug number, but is required
>>        to be able to run and pass all the tests associated with
>> "-testset hotspot"
>>        (for JFR tests the VM can't be started without that).
>>       These will be pushed under this sub-task:
>> https://bugs.openjdk.java.net/browse/JDK-8098588
>>
>> (4) Additional notes:
>> (a) Some pieces of code, only necessary for 1.5 or older,  were removed
>> (b) update_version and special_update_version were removed
>> (c) The code to parse the version string in
>> jdk/test/sun/misc/Version/Version.java
>>        will probably change when the Version.java class is available.
>> (d) As with the changes for 8085822, this is all being pushed to [1] and
>> then later to jdk9/dev
>> (e) These changes should address most, if not all, of the issues raised
>> during
>> the code review for JDK-8085822 (see also
>> https://bugs.openjdk.java.net/browse/JDK-8087203)
>> (f) All  builds and tests pass for "-testset hotspot".
>>
>> [1] http://hg.openjdk.java.net/verona/stage
>>
>> Thanks
>>

-- 
Alejandro