RFR(XS): JDK-8171318 - serviceability/sa/jmap-hprof/JMapHProfLargeHeapTest.java fails latest Jigsaw integration

[serguei.spitsyn at oracle.com]

Hi George,


It looks good but I have a question.
Why the option --add-opens is not passed now?
Does it mean enabling deep reflection is not needed anymore?

Thanks,
Serguei



On 12/21/16 13:36, George Triantafillou wrote:
>
> Hi David,
>
> New webrev: 
> http://cr.openjdk.java.net/~gtriantafill/8171318/webrev.01/ 
> <http://cr.openjdk.java.net/%7Egtriantafill/8171318/webrev.01/>
>
> Tested locally on Linux.
>
> More details inline:
>
> On 12/19/2016 11:13 PM, David Holmes wrote:
>
>> Hi George,
>>
>> On 20/12/2016 1:30 AM, George Triantafillou wrote:
>>> Please review this small change to fix test failures in
>>> JMapHProfLargeHeapTest.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8171318
>>> Open webrev: http://cr.openjdk.java.net/~gtriantafill/8171318/webrev/
>>> <http://cr.openjdk.java.net/%7Egtriantafill/8171318/webrev/>
>>
>> Why is this webrev based against jdk9/jdk9 instead of jdk9/hs?
> The webrev is based on jdk9/hs.  My clone script needs to updated.
>>
>>> JMapHProfLargeHeapTest.java: added add-opens to ProcessBuilder, added
>>> input check for largeHeapScanner.
>>
>> I can't make the existing test fail. I would have thought that 
>> --add-exports subsumed the need to do --add-opens? 
> Actually, --add-opens is required in addition to --add-exports to 
> enable deep reflection.
>> I've been trying to find any doc to clarify that. But it seems to me 
>> that this test as-is doesn't seem to fail since the most recent 
>> Jigsaw refresh.
> With the latest jdk9, the test will always fail with a timeout, which 
> is masked by the lack of an input check for largeHeapScanner. The 
> actual error message follows:
>
> Exception java.lang.reflect.InaccessibleObjectException: Unable to 
> make field private final sun.management.VMManagement 
> sun.management.RuntimeImpl.jvm accessible: module java.management does 
> not "opens sun.management" to unnamed module
>
> Christian pointed out in an offline discussion that a better solution 
> uses jdk.test.lib.process.ProcessTools.getProcessId() in 
> JMapHProfLargeHeapProc.java, which removes the call to 
> setAccessible().  Here's the new webrev:
>
> New webrev: 
> http://cr.openjdk.java.net/~gtriantafill/8171318/webrev.01/ 
> <http://cr.openjdk.java.net/%7Egtriantafill/8171318/webrev.01/>
>
> Thanks.
>
> -George
>
>>
>> Thanks,
>> David
>>
>>> Tested locally on Linux.
>>>
>>> Thanks.
>>>
>>> -George
>>>
>