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

[David Holmes]

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
>