RFR 7041262: VM_Version should be called instead of Abstract_VM_Version so that overriding works

[Harold David Seigel]

Hi David,

Thanks for reviewing this and for catching the ARM32 issue.  I'll make 
the vm_info_string() changes before pushing the change.

Harold


On 10/16/2018 1:14 AM, David Holmes wrote:
> Hi Harold,
>
> On 13/10/2018 1:31 AM, Harold David Seigel wrote:
>> Hi,
>>
>> Please review this fix for JDK-7041262.  The fix changes calls to 
>> Abstract_VM_Version functions to instead call VM_Version functions.  
>> See details in the bug.
>
> There is an issue with the vm_info_string() changes.
>
> On ARM32 we have an "overriding" version of 
> VM_Version::VM_info_string() which will now get called from 
> vmError.cpp. I think the ARM version can be deleted - it was only put 
> in place to add "experimental" to features used in the ARM build, but 
> that has all been superseded now.
>
> In thread.cpp you changed the comment:
>
> !   // is initially computed. See Abstract_VM_Version::vm_info_string().
> !   // is initially computed. See VM_Version::vm_info_string().
>
> but the code you are being referred to actually is defined in 
> Abstract_VM_version::vm_info_string().
>
> ---
>
> We now call VM_Version::foo() all the time instead of 
> Abstract_VM_Version::foo() even though foo() is only "overridden" in a 
> handful of cases, and sometimes there is no base version! I think in 
> places we only ever want to have one definition of foo() - e.g. the 
> main version functions:
>
> static int vm_major_version()               { return _vm_major_version; }
>   static int vm_minor_version()               { return 
> _vm_minor_version; }
>   static int vm_security_version()            { return 
> _vm_security_version; }
>   static int vm_patch_version()               { return 
> _vm_patch_version; }
>   static int vm_build_number()                { return 
> _vm_build_number; }
>
> should not, in all probability, ever be "overridden". In that case we 
> really should call them via Abstract_VM_Version::foo(). But there's no 
> way to express methods that should not be "overridden". :( I think the 
> real answer here (not for this change) is to handle VM_Version the way 
> we handle the os class, and to only generate a single class containing 
> both shared and platform-specific components. That might also allow us 
> to do away with the "parallel work threads" mess:
>
> static unsigned int nof_parallel_worker_threads(...);
> static unsigned int parallel_worker_threads();
> static unsigned int calc_parallel_worker_threads();
>
> Huh! ???
>
> ---
>
> For this change if you address the vm_info_string issues I flagged 
> that suffices - thanks.
>
> I think I may adapt the JBS issue for handling VM_Version_ext so that 
> it collapses all this into a single class.
>
> Thanks,
> David
> -----
>
>
>> Open Webrev: 
>> http://cr.openjdk.java.net/~hseigel/bug_7041262/webrev/index.html
>>
>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-7041262
>>
>> The fix was tested by running Mach5 tiers 1 and 2 tests and builds on 
>> Linux-x64, Windows, and Mac OS X, running tiers 3-5 tests on 
>> Linux-x64, and by running JCK-12 Lang and VM tests on Linux-x64.  I 
>> also did test builds on zero, minimal, and aarch64.
>>
>> Thanks, Harold
>>