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