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
