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

[Frederic Parain]

Looks good to me, thank you for fixing this.

Fred

> On Mar 24, 2017, at 03:28, Chris Plummer <chris.plummer at oracle.com> wrote:
> 
> On 3/23/17 11:59 PM, Chris Plummer wrote:
>> On 3/23/17 11:41 PM, David Holmes wrote:
>>> 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 :)
>>> 
>> Sorry, I forgot about that. I'll get that fixed.
> Updated with new methods/fields made private instead of public:
> 
> http://cr.openjdk.java.net/~cjplummer/8177055/webrev.02/webrev.hotspot/
> 
> thanks,
> 
> Chris
>> 
>> Chris
>>> 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
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>>> 
>> 
>