NEED REVIEWER Re: RFR(S): JDK-8151991 jvmti diagnostics commands requires INCLUDE_SERVICES

[serguei.spitsyn at oracle.com]

Looks good.

Thanks!
Serguei


On 3/24/16 04:48, Dmitry Samersoff wrote:
> Serguei,
>
> Webrev updated in-place (press shift-reload):
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8151991/webrev.02/
>
> -Dmitry
>
>
> On 2016-03-24 12:22, serguei.spitsyn at oracle.com wrote:
>> Hi Dmitry,
>>
>> -#if INCLUDE_SERVICES // Heap dumping/inspection supported
>> +#if INCLUDE_SERVICES
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<HeapDumpDCmd>(DCmd_Source_Internal | DCmd_Source_AttachAPI, true, false));
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<ClassHistogramDCmd>(full_export, true, false));
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<ClassStatsDCmd>(full_export, true, false));
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<ClassHierarchyDCmd>(full_export, true, false));
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<SymboltableDCmd>(full_export, true, false));
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<StringtableDCmd>(full_export, true, false));
>> + DCmdFactory::register_DCmdFactory(new
>> DCmdFactoryImpl<JVMTIAgentLoadDCmd>(full_export, true, false));
>>   #endif // INCLUDE_SERVICES
>>   #if INCLUDE_JVMTI
>>     DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<JVMTIDataDumpDCmd>(full_export, true, false));
>> - DCmdFactory::register_DCmdFactory(new
>> DCmdFactoryImpl<JVMTIAgentLoadDCmd>(full_export, true, false));
>>   #endif // INCLUDE_JVMTI
>>
>>
>> The registration of the JVMTIAgentLoadDCmd has to be guarded with the INCLUDE_JVMTI.
>> And now, it is not guarded.
>>
>> Otherwise, the fix looks good.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>>
>>
>> On 3/23/16 10:43, Dmitry Samersoff wrote:
>>> Everybody,
>>>
>>> Webrev updated according to David's concerns.
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8151991/webrev.02/
>>>
>>> -Dmitry
>>>
>>> On 2016-03-18 07:42, David Holmes wrote:
>>>> On 17/03/2016 12:14 AM, Dmitry Samersoff wrote:
>>>>> Everybody,
>>>>>
>>>>> Please, review small fix.
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8151991/webrev.01/
>>>>>
>>>>> New diagnostic command (JVMTI.agent_load) should be guarded by
>>>>> #if INCLUDE_SERVICES and don't brake minimal VM build.
>>>> Initially I was confused as to why this was associated with
>>>> INCLUDE_SERVICES as it seems unrelated to the all the other things
>>>> guarded by INCLUDE_SERVICES. But now I see that the attachListener code
>>>> is completely excluded by INCLUDE_SERVICES (excludeSrc.gmk) so it makes
>>>> sense that the same guard is used in the diagnosticCommand sources (or
>>>> else an independent guard introduced?).
>>>>
>>>> However you would then also need the same guard in:
>>>>
>>>> src/share/vm/prims/jvmtiExport.cpp
>>>>
>>>> to allow INCLUDE_JVMTI to be true and INCLUDE_SERVICES to be false.
>>>>
>>>> lso can you update this comment:
>>>>
>>>>   64 #if INCLUDE_SERVICES // Heap dumping/inspection supported
>>>>
>>>> to also refer to the JVM TI agent/attach support
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>
>>>>> -Dmitry
>>>>>
>