RFR (S): 8022452: Hotspot needs to know about Windows 8.1 and Windows Server 2012 R2

[Mikael Vidstedt]

Coleen,

After having tried numerous different versions I ended up with this:

http://cr.openjdk.java.net/~mikael/webrevs/8022452.3/webrev

Summary of the changes:

* The GetNativeSystemInfo call has been moved up before the switch, but 
is only called if the os version is recent enough, and  we actually care 
about the si.wProcessorArchitecture field.

* The true/else branches in the if-diamond have been switch to avoid the 
negation of the condition.

* While at it I added support for recognizing "Windows Server 2008 R2" 
(6.1 non-workstation) which we apparently haven't recognized earlier, 
but which is something the JDK code recognizes [1].

* The " , 64 bit" has been move out below the switch, but is only 
relevant/tested for on recent enough Windows versions, just like the old 
code did.

Feedback much appreciated! I'm also investigating if it would be 
possible to unify/share the code with the java_props_md.c logic [1] to 
avoid the duplication, but for now this may have to do. In addition to 
that the code can be cleaned up further if we only support VS2010+, 
which I believe is de facto since JDK 7 anyway; I will investigate that 
as a separate improvement.

Thanks,
Mikael

[1] 
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/e057cddf0d6c/src/windows/native/java/lang/java_props_md.c 
(line 451).

On 2013-08-07 10:56, Coleen Phillimore wrote:
>
> Mikael,
>
> I do like your new version better but because you changed so much, it 
> should be verified on all versions of windows.   Which you might not 
> want to do.   My suggestion to make this code nicer (fix case stmt and 
> outline common code) was really simple and I think can be verified by 
> code inspection, which might be less work and easier to review.
>
> Coleen
>
> On 8/7/2013 12:28 PM, Mikael Vidstedt wrote:
>>
>> Coleen,
>>
>> Funny you should mention that, and since I fully agree with you I 
>> actually did prepare another version of the patch which is heavily 
>> inspired by the java_props_md.c implementation:
>>
>> http://cr.openjdk.java.net/~mikael/webrevs/winvercleanup2/webrev/
>>
>> I personally like this version better, but the one thing that held me 
>> back was the fact that I will basically want to dig up machines with 
>> all the different (supported) Windows versions and verify that the 
>> code is still correct in all cases. If you think this is the way to 
>> go I more than willing to will do so. Let me know what you think 
>> about the new version of the patch!
>>
>> There's no real urgency here - I'd rather do it correct once than...
>>
>> Cheers,
>> Mikael
>>
>>
>> On 2013-08-07 06:39, Coleen Phillimore wrote:
>>>
>>> It looks like the code under the case for all these releases, should 
>>> be different case statements for each, rather than these cascading 
>>> 'if' statements.  And make 1668-1673 a new function returning si.   
>>> So if we have a new release, in general it will work because the 
>>> default will print out something.   But a better default would be 
>>> lines 1712-1717 which are dead code now.
>>>
>>> Sorry, I know it's simple and urgent but this seems to be getting 
>>> increasingly ugly.
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 8/6/2013 7:36 PM, Mikael Vidstedt wrote:
>>>>
>>>> Please review the following change:
>>>>
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8022452/webrev
>>>>
>>>> The patch adds support to os_windows.cpp for recognizing the 
>>>> upcoming Windows versions: Windows 8.1 and Windows Server 2012 R2.
>>>>
>>>> The changed is based on the corresponding code on the JDK 
>>>> side[1][2][3].
>>>>
>>>> Thanks,
>>>> Mikael
>>>>
>>>> [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8020191
>>>> [2] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/40221b09812f
>>>> [3] 
>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019463.html
>>>>
>>>
>>
>