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]

Thanks Fred.

Chris

On 3/24/17 6:00 AM, Frederic Parain wrote:
> 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
>>>>>>>>>>>>