8161605 : The '!UseSharedSpaces' check is not need in JvmtiManageCapabilities::recompute_always_capabilities

[Mikhailo Seledtsov]

Hi Alex,

   Several test-related comments:
   - CanGenerateAllClassHook.java
      " 44  * So the test runs 2 java process": s/process/process*es*/

   - CanGenerateAllClassHook.java
      A general comment - please consider using "TestCommon" class and 
"CDSTestUtils" where possible.
      There are useful methods for forming ProcessBuilder with 
parameters specific to AppCDS, and also for checking results.
      In some cases the test case is so custom that use of these 
utilities is not applicable.
      Please take a look at ClassFileLoadHookTest.java as an example.  
It uses TestCommon.executeAndLog(),
      TestCommon.checkExec() and more. Other good examples are 
SharedArchiveFile.java, HelloExtTest.java or other tests under
      appcds directory.

      Important to note a condition of "mapping failure". When using 
CDS/AppCDS
      with Xshare:on, the mapping of a CDS archive to memory may fail at 
random (due to ASLR or other conditions).
      TestCommon.checkExec() handles such conditions propertly, 
otherwise you will have random/intermittent test failures.

   - CanGenerateAllClassHook.java

     122     private static boolean isWin() {

      I recommend using jdk.test.lib.Platform, the isWindows() method. 
The SQE guideline is to use test utilities where possible.
      FYI, the test utilities reside in: open/test/lib/jdk/test/lib


The rest of the tests look good,

Thank you,
Misha


On 2/1/18, 4:11 PM, Alex Menkov wrote:
> Hi Serguei,
>
> Thanks for the feedback.
>
> Updated fix: 
> http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev.02/
>
> --alex
>
> On 02/01/2018 11:00, serguei.spitsyn at oracle.com wrote:
>> One more attempt to send it with the correct to-list...
>> Sorry for the noise.
>>
>>
>> Hi Alex,
>>
>> It looks good in general.
>> A couple of comments on the test.
>>
>> We had a convention to avoid having test sub-folders with bug numbers.
>> Could you, please, rename it to some symbolic name?
>>
>> Also we need the test to fail in the native agent when an error is 
>> reported with the reportError().
>>
>> It'd be great if some CDS/AppCDS experts from the Runtime team look 
>> at the fix and test
>> so I've included Jiangli and Misha into the to-list.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 1/25/18 16:11, Alex Menkov wrote:
>>> Hi all,
>>>
>>> Please take a look at a fix for
>>> JDK-8161605 : The '!UseSharedSpaces' check is not need in 
>>> JvmtiManageCapabilities::recompute_always_capabilities
>>>
>>> The fix moves can_generate_all_class_hook_events capability from 
>>> "onload_capabilities" to "always_capabilities" and cleans up 
>>> recompute_always_capabilities method.
>>>
>>> jira: https://bugs.openjdk.java.net/browse/JDK-8161605
>>> webrev: 
>>> http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev/
>>>
>>> --alex
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180202/1a509df6/attachment.html>