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]

Hello,

Can I get a second reviewer please. Here's an updated webrev that fixes 
the issue with isSupportedPlatform being declared both as a static and 
as a local. The local declaration was removed:

http://cr.openjdk.java.net/~cjplummer/8177055/webrev.01/webrev.hotspot/

thanks,

Chris

On 3/23/17 7:35 PM, Chris Plummer wrote:
> 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
>>>>>>>
>>>>>>>
>>>>>
>>>
>