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

[George Triantafillou]

Hi Serguei,

On 12/21/2016 5:59 PM, serguei.spitsyn at oracle.com wrote:
> 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?
Deep reflection is no longer needed since the call to setAccessible() in 
JMapHProfLargeHeapProc.java was removed.  It was replaced with 
ProcessTools.getProcessId(), which uses ProcessHandle.current().getPid() 
to get the current process ID.

-George

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