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]

On 24/03/2017 4:24 PM, Chris Plummer wrote:
> 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/

Okay on the updates. But I'd still like to not see everything declared 
public :)

Thanks,
David

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