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

[Chris Plummer]

On 3/23/17 6:57 PM, David Holmes wrote:
> 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.
>
I guess I just don't think "dynamic" and "static" are appropriate here. 
I think I'll keep it the way it is.

Thanks for the review,

Chris
> 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
>>>>>>
>>>>>>
>>>>
>>