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