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

   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.

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