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

[serguei.spitsyn at oracle.com]

Hi Alex,

It looks good.
Just a couple of minor comments though.

  109             log("onLoad = " + onLoadValue);
  110             log("live= " + liveValue);
  111             if (onLoadValue != liveValue || onLoadValue < 0) {
  112                 throw new RuntimeException("Inconsistent can_generate_all_class_hook_events value");
  113             }


It'd be nice to make more clear statement about why the test has been 
failed.
A couple of things to cover:
   - the capability is expected to be available in both phases,
     but now the test would pass if both phases returned false
   - an unexpected difference in returned capability in onload and live 
phases
   - what does it mean if the printed capability value is negative

Something like the following would work:
   log("can_generate_all_class_hook_events capability values returned in 
ONLOAD and LIVE phases:");
   log("ONLOAD phase: " + (onLoadValue < 0 ? "Failed to read" : 
onLoadValue);
   log("LIVE phase:   " + (liveValue   < 0 ? "Failed to read" : liveValue);

   if (onLoadValue != 1 || liveValue != 1) {
       throw new RuntimeException("The can_generate_all_class_hook_events "
             " expected to be available in ONLOAD and LIVE phases");
   }

It is better to remove the line:

  145         //log("CommandLine: " + params.stream().collect(Collectors.joining(" ")));


Thanks,
Serguei


On 2/1/18 16:11, 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
>>