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]

Hi David,

On 3/23/17 2:40 AM, David Holmes wrote:
> On 23/03/2017 6:08 PM, Chris Plummer wrote:
>> Hi David,
>>
>> On 3/23/17 12:43 AM, David Holmes wrote:
>>> Hi Chris,
>>>
>>> Is there no property we can query that will give a different value for
>>> the two ports? Could we easily add anything? What does
>>> -Xinternalversion show? (though I suppose checking that is no
>>> different to checking how it responds to -XX:StackReservedPages=1)
>> Bob, Andrew and I discussed this some, and neither of them knew of a
>> property to query. I haven't tried -Xinternalversion. I'll try for the
>> Oracle port tomorrow. Andrew will need to tell me what the Open AArch64
>> port says. However, even if this info could be used, I don't think it
>> would be a good idea. With my current changes, if any port adds or
>> removes this feature the test will detect it and you'll get some sort of
>> error message. If we start looking at version strings, you don't get
>> that benefit.
>
You probably have seen now the new bug filed to make is so you can 
differentiate between the two versions of Aarch64 ports. See 
https://bugs.openjdk.java.net/browse/JDK-8177390. 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.
> 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.
>
>>>
>>> That aside some issues with your logic:
>>>
>>> 202             if (isSupportedPlatform && 
>>> !result.contains("PASSED")) {
>>>
>>> 251     public static boolean isSupportedPlatform;
>>>
>>> 266         boolean isSupportedPlatform = true;
>>>
>>> 270             isSupportedPlatform = false;
>>>
>>> Line #202 is reading the static variable declared at line #251. But
>>> line #266 introduces a local variable with the same name, which may be
>>> set at line #270. So you never initialize the static AFAICS.
>> Nice catch. That happened because I had the code elsewhere at one point
>> with a local instead of a static. So that means despite my sanity
>> checking to make sure I didn't disabled the test for ports that support
>> the reserved stack area, I still managed to break the test. :(
>>>
>>> ---
>>>
>>> Nit: none of these static fields/members need to be public.
>> Ok.
>>>
>>> ----
>>>
>>> 202             if (isSupportedPlatform && 
>>> !result.contains("PASSED")) {
>>>
>>> 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.
>
>>   206                 // Either the test passed or this platform is not
>> supported.
>>  207                 // On not supported platforms, we only expect the
>> VM to
>>  208                 // not crash during the test. This is especially
>> important
>>  209                 // on Windows where the detection of SOE in 
>> annotated
>>  210                 // sections is implemented but the reserved zone
>> mechanism
>>  211                 // to avoid the corruption cannot be implemented 
>> yet
>>  212                 // because of JDK-8067946
>>>
>>>  247     public static boolean isNeverSupportedPlatform() {
>>>  248         return !isAlwaysSupportedPlatform() &&
>>> !Platform.isAArch64();
>>>  249     }
>>>
>>> I don't understand what platform this is trying to isolate ??
>> Need to look here also:
>>
>>  281         // And some platforms we know are never supported. Make 
>> sure
>>  282         // we didn't determine that one of those platforms is
>> supported.
>>  283         if (isSupportedPlatform && isNeverSupportedPlatform()) {
>>  284             String msg  = "This platform should not be supported: "
>> + Platform.getOsArch();
>>  285             System.err.println("FAILED: " +  msg);
>>  286             throw new RuntimeException(msg);
>>  287         }
>>
>> It's just making sure that isSupportedPlatform didn't get set true for a
>> platform we know should not be supporting this feature. AArch64 is the
>> only platform that may or may not support the feature. We have a list of
>> platforms that always support it. All other platforms except for AArch64
>> are expected to not support it.
>
> 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.

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