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

[serguei.spitsyn at oracle.com]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160324/ffeec303/attachment.html>