RFR(10)(S) 8177055: [TESTBUG] test/runtime/ReservedStack/ReservedStackTest.java sometimes fails on Oracle arm64 port after being enabled for open aarch64 port

[David Holmes]

Hi Chris,

<trimming>

On 24/03/2017 4:32 AM, Chris Plummer wrote:
> On 3/23/17 2:40 AM, David Holmes wrote:
> However, I still don't
> think that's the way to go for detecting if a feature is present,
> especially if which ports do and do not support the feature can change.

Agreed - an explicit feature test is best as I said.

>> That's true. A direct feature test is better. Maybe a WhiteBox API
>> addition?
> That's possible, but I don't see any other cases of WB telling you if a
> feature is present. The closest I found is getting the supported GCs.
> Also, it seems to be a bit of overkill  for something that has an
> alternate simpler solution.

At one stage I thought we were looking at using it for different 
capabilities that may not be present in the minimal VM, but I think we 
ended up just using that information via @requires in the tests. But the 
GC support is a good example. I don't see it as overkill at all - and a 
lot less klunky than exec'ing another VM instance and parsing stderr!

But it is just a suggestion. We do exec and parse a lot of VMs in testing.

>>>> Why does the test check for a supported platform here instead of at
>>>> the start of main (or in your case in initIsSupportedPlatform()) ??
>>> That's the way the test has always worked:
>>
>> Yep realized that but have no idea why. Normally you check for the
>> validity of a test at the beginning in main(). Seems really odd to run
>> the test and then if it fails checks to see if we expected it to pass.
> The point is the test still has a purpose on unsupported platforms. It's
> an SOE stress test.

Ah I see. Had not realized that. Seems a somewhat obscure property of 
the test not evident in its name.

>> So we have three logical tests:
>> - Always supported
>> - Sometimes supported (AArch64 only)
>> - Never supported
>>
>> And Never := !Always && !Sometimes
> Yes.
>>
>> Okay. Might be clearer to classify as "statically supported" and
>> dynamically supported". Or not.
> You mean just rename the APIs?  Adding "static" seems kind of clumsy
> since you still need "never" and "always". So you get
> isStaticallyNeverSupportedPlatform, isStaticallyAlwaysSupportedPlatform,
> and isDynamicallySometimesSupportedPlatform???  I doubt this is what you
> are looking for. Maybe you had something else in mind.

isStaticallySupportedPlatform() { return isx86() || issparc() || ...}
isDynamicallySupportedPlatform() { return isAarch64(); }
isNeverSupportedPlatform() { return !isStaticallySupportPlatform() && 
!isDynamicallySupportedPlatform(); }

Though Platform is somewhat unnecessary and could be dropped.

Again just a suggestion - feel free to ignore.

Cheers,
David

> thanks,
>
> Chris
>>
>> Thanks,
>> David
>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>> On 23/03/2017 4:52 PM, Chris Plummer wrote:
>>>>> Ping!
>>>>>
>>>>> On 3/21/17 6:15 PM, Chris Plummer wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8177055
>>>>>> http://cr.openjdk.java.net/~cjplummer/8177055/webrev.00/webrev.hotspot
>>>>>>
>>>>>>
>>>>>> The test was recently enabled for aarch64 platforms since support for
>>>>>> stack reserved pages was added to the open aarch64 port. However, the
>>>>>> Oracle port still does not have this feature, causing the test to
>>>>>> sometimes fail when run on it. Note the test is always run on all
>>>>>> platforms. However, when run on an unsupported platform,
>>>>>> "failures" do
>>>>>> not result in the test actually failing (although crashes will). This
>>>>>> is intentional and is how the test is suppose to work.
>>>>>>
>>>>>> The fix is to change how the test determines if it is running on a
>>>>>> supported platform. It used to use a fixed list of supported
>>>>>> platforms. Now I have it spawn a subprocess with
>>>>>> "-XX:StackReservedPages=1" to see if it produces the warning message
>>>>>> you should see on unsupported platforms:
>>>>>>
>>>>>> WARNING: Reserved Stack Area not supported on this platform
>>>>>>
>>>>>> If it doesn't appear, then the platform is assumed to support the
>>>>>> feature. I still use the old list of supported platforms (minus
>>>>>> aarch64) for sanity checks. Basically all platforms in the old list
>>>>>> (except aarch64) should never show the above warning, and all
>>>>>> platforms not in the old list (except aarch64) should always show the
>>>>>> above warning.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>
>>>>>
>>>
>