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

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