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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.