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

David Holmes david.holmes at oracle.com
Thu Mar 23 09:40:58 UTC 2017


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.

That's true. A direct feature test is better. Maybe a WhiteBox API addition?

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

>   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

Okay. Might be clearer to classify as "statically supported" and 
dynamically supported". Or not.

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


More information about the hotspot-runtime-dev mailing list