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

[Alex Menkov]

Thank you for the feedback.

Updated fix:
http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev.03/

--alex

On 02/02/2018 11:18, Mikhailo Seledtsov wrote:
> 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
>>>