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

[David Holmes]

I retract my Review.

Kim is right: no second call to actually get the right vectors into the 
alloca'd array; and nothing in the manpage that says getisax works that 
way when you pass NULL!

David

On 23/09/2016 11:08 AM, Kim Barrett wrote:
>> On Sep 22, 2016, at 6:54 PM, Alan Burlison <Alan.Burlison at oracle.com> wrote:
>>
>> getisax(2) and meminfo(2) are both Committed interfaces. getisax(2) first appeared in Solaris 10, meminfo(2) first appeared in Solaris 9. Currently they are both accessed via dlsym(3C) which is unnecessary as they will always be present on any version of Solaris that Java9+ will run on. In addition, this patch modifies the use of getisax(2) to probe for the correct buffer size to use so it will continue to work if/when the current 2-word buffer size is exceeded - although of course any newly-returned capability bits will need handling, as is already the case.
>>
>> I'm submitting these two bugs as a single unit for review as they share common code (os::Solaris::misc_sym_init()) which can be removed if they are fixed together.
>>
>> 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/
>>
>> JPRT hostpot testset is clean.
>>
>> --
>> Alan Burlison
>> --
>
> ------------------------------------------------------------------------------
> src/os_cpu/solaris_sparc/vm/vm_version_solaris_sparc.cpp
>  360   uint_t avn = getisax(NULL, 0);
>  361   uint_t *avs = (uint_t*) alloca(avn * sizeof(uint_t));
>  362   uint_t av = avs[0];
>
> I see nothing here to fill in the alloca'ed avs array.  Shouldn't there
> be another call to getisax?
>
>> JPRT hostpot testset is clean.
>
> That suggests the conditionalization on all these features is pretty
> good, since it looks to me like that testing didn't use real values.
>
> The use of getisax with an empty array to determine how many elements
> to allocate for a real call doesn't obviously follow from the
> referenced documentation.  "... returns the number of array elements
> that contain non-zero values." suggests to me that it should be no
> greater than the "n" (second) argument and could be less, as it is the
> number of non-zero elements in the first (array) argument that is
> returned. Perhaps that's a bug in the documentation?
>
> I think the old code that only supported two elements was fine, except
> needing to replace os::solaris::getisax with just getisax. Getting the
> actual length and using alloca seems like overkill to me. We're not
> looking for any entries beyond the second element. If we ever want to
> start doing so, just increase the static length accordingly.  This is
> assuming the behavior exhibited by the empty array call, e.g. the
> result is the number of entries that are available for access, and not
> limited by the size of the array argument.
>
> However, the old assertion that the result of getisax <= 2 *is*
> inconsistent with the behavior that seems to be expected from using an
> empty array to get the available number of entries.
>
> ------------------------------------------------------------------------------
>