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/


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?



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?)



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.



I think a lot of the modified code is obsolete post Hotspot Express - 
which makes it hard to determine if the changes make sense.



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) 



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.



This comment is nonsensical:

!      * Returns the security version of the running JVM if it's 1.6 or 
        * 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.


> 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 compiler-dev mailing list