RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)
David Holmes
david.holmes at oracle.com
Wed Jun 10 09:58:11 UTC 2015
Hi Magnus,
Generally looks good - a few comments/queries below.
On 9/06/2015 11:52 PM, Magnus Ihse Bursie wrote:
> Here's an updated webrev, which fixes the typos that were pointed out by
> reviewers:
> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.02/
common/autoconf/version-numbers
Shouldn't there be a DEFAULT_VERSION_XXX for each of the XXX parts of
the version info? (Even if all zeroes presently.)
---
Looking at hotspot changes I can't convince myself that the new
streamlined "version" variables will capture things like "fastdebug".
Will that filter through via configure variables?
---
make/*/makefiles/vm.make
Shouldn't the -DVERSION_XX=$(VERSION_XX) definitions be taken from the
VERSION_CFLAGS in spec.gmk? (Or are you still allowing for a stand-alone
hotspot build?)
---
hotspot/src/share/vm/prims/jvm.h
jdk/src/java.base/share/native/include/jvm.h
I think this comment is way out of date:
/* Build number is available only for RE build (i.e. JDK_BUILD_NUMBER
is set to bNN)
* It will be zero for internal builds.
*/
and can be completely removed. Even if still true, mention of an "RE
build" has no place in OpenJDK sources.
---
hotspot/src/share/vm/runtime/java.cpp
I think a lot of the modified code is obsolete post Hotspot Express -
which makes it hard to determine if the changes make sense.
---
hotspot/src/share/vm/runtime/vmStructs.cpp
Please fix the alignment (3 extra spaces on modified line):
1239 static_field(Abstract_VM_Version, _vm_minor_version,
int) \
1240 static_field(Abstract_VM_Version,
_vm_security_version, int)
\
---
hotspot/test/runtime/6981737/Test6981737.java
This test is really only valid for the new version scheme now, so it
should probably be rewritten - and in that case it really isn't testing
6981737 but should be replaced by a new test for the new version format.
---
jdk/src/java.base/share/classes/sun/misc/Version.java.template
This comment is nonsensical:
/**
! * Returns the security version of the running JVM if it's 1.6 or
newer
* or any RE VM build. It will return 0 if it's an internal 1.5 or
* 1.4.x build.
* @since 1.6
*/
as security version does not exist pre 9. Normally you should be adding
a new method and deprecating the old one. The new one is @since 9.
/**
! * Returns the security version of the running JDK.
* @since 1.6
*/
Ditto: @since 9 (but again old should be deprecated and new method added)
253 /**
254 * Returns the build number of the running JDK if it's a RE build
255 * It will return 0 if it's an internal build.
As with jvm.h this seems obsolete commentary these days - not only RE
builds define a build number.
Thanks,
David
>
> And here's a (much simpler) delta webrev which shows just these changes:
> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch-delta-01/webrev.01/
>
>
> /Magnus
>
> On 2015-06-09 15:20, Daniel D. Daugherty wrote:
>> On 6/9/15 7:12 AM, Magnus Ihse Bursie wrote:
>>> Hi Daniel,
>>>
>>> Thank you for your thorough review!
>>
>> This was my (failing) attempt at a "fast pass" review... :-)
>>
>>
>>>
>>> On 2015-06-09 01:31, Daniel D. Daugherty wrote:
>>>> >
>>>> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.01
>>>>
>>>>
>>>> General comment: Not all copyright years were updated.
>>>> General comment: It looks like support for the 'patch' value is not
>>>> completely
>>>> implemented through all the Makefiles. I didn't audit for this,
>>>> but it's
>>>> just my impression.
>>>
>>> You are basically correct. The makefiles all propagate the patch
>>> value where needed, but the actual source code changes in hotspot and
>>> jdk ignores the patch field as of now. This will be corrected in a
>>> later add-on patch, submitted by someone from the jdk and/or hotspot
>>> team rather than the build team.
>>>
>>>>
>>>> common/autoconf/generated-configure.sh
>>>> There are multiple Copyright notices in this file. Why?
>>> Oh dear, you've reviewed the generated file. :-& I'm really impressed
>>> by your effort!
>>
>> Ahhh... now that you say it... it sounds familiar... I may have
>> made this same mistake before. I'll try to remember next time.
>>
>>
>>> However, the generated-configure.sh file is automatically created by
>>> the autoconf framework from the rest of the files in
>>> common/autoconf/*, so we cannot direcly modify it's contents - only
>>> indirectly. The reason it's checked in, is that otherwise every user
>>> would need to generate it before being able to run configure. In
>>> build reviews, we usually either revert changes to
>>> generated-configure.sh before posting a review to hide it (and
>>> re-generate it before pushing), or we just ignore it during reviews.
>>> I should have done that, or pointed out that it was not needed nor
>>> possible to review when I cross-posted. I'm sorry.
>>>
>>>>
>>>> L4076: # Verify that a given string represent a valid version
>>>> number, and assing it to
>>>> L4077: # a variable.
>>>> Fixed two typos and reformat a bit:
>>>> # Verify that a given string represents a valid version
>>>> number, and
>>>> # assigning it to a variable.
>>> I'll put that fix in the source .m4 file instead. Thanks.
>>>>
>>>> L20262: as_fn_error $? "--with--version-string must have a
>>>> value" "$LINENO" 5
>>>> The '--with--version...' part doesn't match previous usages
>>>> where
>>>> '--with-version...' is used
>>> Yes, you're right. Erik pointed it out as well. It's a typo in the
>>> error message. It's all over the place due to copied code. I'll fix
>>> it in the source .m4 file.
>>>
>>> (As a side note, I have a half-finished follow-up patch that will
>>> reduce the amount of code duplication, but it requires some framework
>>> changes so it'll have to be a separate thing.)
>>>
>>>>
>>>> L20275: # Unspecified numerical fields is interpreted as 0.
>>>> Grammar: 'is interpreted' -> 'are interpreted'
>>>>
>>>> L20286: as_fn_error $? "Version string contains + but
>>>> both 'BUILD' and 'OPT' is missing" "$LINENO" 5
>>>> Grammar: 'is missing' -> 'are missing'
>>> Those darn English plural forms! Why can't you all do as we sensible
>>> Swedes and get rid of them? ;-)
>>>
>>> (I'll fix)
>>>
>>>>
>>>> L20388: username=`$ECHO "$USER" | $TR -d -c
>>>> '[a-z][A-Z][0-9]'`
>>>> This assumes that the "USER" variable is set. Should there
>>>> be a check for "" and perhaps use "no_user_specified" or
>>>> something like that? Perhaps the build setup always makes
>>>> sure that "USER" is set to something. Don't know.
>>> Hm. I think the worst thing that'll happen is that the username part
>>> of the opt string gets empty. This part is basically copied right off
>>> the old build, where we have relied on the $USER variable being
>>> present for all the time with no problems, so I think it's quite safe
>>> to assume.
>>>>
>>>> common/autoconf/jdk-options.m4
>>>> Don't know why the 'elliptic curve crypto implementation' support
>>>> is relocated as part of this changeset, but ...
>>> It was incorrectly placed not at the top indentation level, but in
>>> the middle of the function definition where the old versioning code
>>> were handled. (Which, mostly by chance, happens to work in autoconf,
>>> but is really bad style).
>>>
>>>> make/Javadoc.gmk
>>>> Did you mean to remove the 'clean' target?
>>> Yep. Cleaning is done by the top-level Main.gmk makefile nowadays,
>>> that's just some dead code.
>>>
>>>>
>>>> hotspot/make/windows/makefiles/compile.make
>>>> No changes in the frames view.
>>>> Update: udiff view shows a blank line deleted at the end of the
>>>> file.
>>>
>>> That's probably the result of some change going forth and then back
>>> again during development. But then again, removing extra blank linkes
>>> is not a bad thing. (jcheck unfortunately does not enforce any style
>>> checks for make files :-( so they tend to detoriate over time.)
>>>
>>>>
>>>> hotspot/src/share/vm/runtime/java.cpp
>>>> L661: void JDK_Version::fully_initialize(
>>>> L662: uint8_t major, uint8_t minor, uint8_t security,
>>>> uint8_t update) {
>>>> L663: // This is only called when current is less than 1.6 and
>>>> we've gotten
>>>>
>>>> Since you're ripping out vestigial version support, I think
>>>> this
>>>> function can go away since this is version 9 and newer.
>>>> Don't really
>>>> know for sure, but based on that comment...
>>> It probably can, yes. This and other core changes to the actual
>>> .cpp/.java files will be addressed in a follow-up patch.
>>>>
>>>> hotspot/src/share/vm/runtime/java.hpp
>>>> No comments.
>>>>
>>>> hotspot/src/share/vm/runtime/vmStructs.cpp
>>>> L1240: please make the 'int' parameter align like the rest.
>>> Are you okay with defering such a change to a follow-up patch?
>>
>> Yes.
>>
>>
>>> It's likely the entire struct will need changes to be able to handle
>>> a theoretically arbitrarily long version number.
>>>
>>>>
>>>> hotspot/src/share/vm/runtime/vm_version.cpp
>>>> L84: void Abstract_VM_Version::initialize() {
>>>> L85: // FIXME: Initialization can probably be removed now.
>>>> I agree, but that would entail also removing the
>>>> _initialized and the uses of it... Follow on bug fix?
>>> Definitely follow on.
>>>
>>>> jdk/src/java.base/share/classes/sun/misc/Version.java.template
>>>> L149: * Returns the security version of the running JVM if
>>>> it's 1.6 or newer
>>>> This JavaDoc update is wrong, but it might not be important
>>>> if sun.misc.Version class is going away.
>>> I'm not sure if it's going to be replaced by, or just complemented by
>>> java.util.Version, but in any case it will need a follow-up patch to
>>> clean up this and other issues.
>>>> langtools/src/java.compiler/share/classes/javax/lang/model/SourceVersion.java
>>>>
>>>> old L171: case "1.9":
>>>> new L171: case "9":
>>>> Should this logic support both versions? Will dropping
>>>> "1.9" here prevent a pre-version changeset JVM from
>>>> being dropped into a JDK for triage purposes?
>>>>
>>>> Granted we don't often triage 'javac' with different JVMs,
>>>> but...
>>> I'll defer that question to Kumar, who wrote that piece of code. My
>>> guess is that when Hotspot express was dropped, the ability to use a
>>> JVM from another JDK release bit rotteded away.
>>>
>>> /Magnus
>>
>> Dan
>
More information about the build-dev
mailing list