RFR: JDK-8157141 & JDK-8166454: Solaris getisax(2) and meminfo(2) cleanup

[David Holmes]

On 29/09/2016 9:15 AM, Kim Barrett wrote:
>> On Sep 27, 2016, at 9:47 AM, Alan Burlison <Alan.Burlison at oracle.com> wrote:
>>
>> On 27/09/2016 00:54, David Holmes wrote:
>>
>>> Thanks Alan. It is the creation of the intermediate log-stream that can
>>> not be accomplished at this stage of VM initialization so we will need
>>> to stick with what you had. Thanks for trying.
>>
>> Done. I've tweaked the output slightly, it is now:
>>
>> torside-kz6$ ./java -Xlog:os+cpu -version
>> [0.177s][info][os,cpu] getisax(2) returned 1 words:
>> [0.177s][info][os,cpu]     word 1: 0x3ffe8df0
>>
>> That's on a M5, on a T7 and similar it will be 2 words. I've respun the webrev, for reference here are the review links again:
>>
>> getisainfo() manpage:
>> http://docs.oracle.com/cd/E53394_01/html/E54765/getisax-2.html
>>
>> meminfo() manpage:
>> https://docs.oracle.com/cd/E53394_01/html/E54765/meminfo-2.html
>>
>> Bugs:   https://bugs.openjdk.java.net/browse/JDK-8157141
>>        https://bugs.openjdk.java.net/browse/JDK-8166454
>> Webrev: http://cr.openjdk.java.net/~alanbur/JDK-8157141%2bJDK-8166454/
>
> ------------------------------------------------------------------------------
> src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp
>  377   uint_t av1 = avs[AV_HW1_IDX];
>  378   if (av1 & AV_SPARC_MUL32)        features |= hardware_mul32_m;
> ...
>
> Good to use the symbolic index AV_HW1_IDX. Bad form to assume we have
> that many entries.  That should be
>
>   if (avn > AV_HW1_IDX) {
>     uint_t av1 = avs[AV_HW1_IDX];
>     if (av1 & AV_SPARC_MUL32)        features |= hardware_mul32_m;
>     ...
>   }

Surely we can just assert avn > 0 after:

  uint_t avn = getisax(NULL, 0);

It should never be able to return anything < 1 regardless.

David
-----

> ------------------------------------------------------------------------------
> src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp
>  368   uint_t avn = getisax(NULL, 0);
>  369   uint_t *avs = (uint_t*) alloca(avn * sizeof(uint_t));
>  370   (void) getisax(avs, avn);
>
> I still think the use of alloca here is overkill, along with depending
> on (currently) undocumented behavior of getisax, but this is called
> once initialization code and you've filed the doc bug, so okay.
>
> ------------------------------------------------------------------------------
>
> I don't need a new webrev for the insertion of the avn check.
>
>> A JPRT hotspot testset run is clean.
>
> Have you used logging to hand-verify the expected features are being set?
>