[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: NEED REVIEWER Re: RFR(S): JDK-8151991 jvmti diagnostics commands requires INCLUDE_SERVICES
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-03-24 12:08:15
Message-ID: 56F3D8AF.7010105 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]