RFR (M) 8214892: Delayed starting of debugging via jcmd

[Langer, Christoph]

Hi Ralf,

thanks for doing this change. Overall, it looks very good to me and seems to be a nice feature.

I have thought about a more generic name for the option rather than "onjcmd". What about "standby=y". That would indicate that the debugging agent is on standby and can be triggered by whatever means. What do others think?

Here are some minor comments, mostly about the wording of text messages:

src/hotspot/share/services/diagnosticCommand.cpp, line 1097:
I'd prefer  if you write out either "Debugging has been started." or "Debugging is already active.". I think this would make it more clear for the user of the feature what actually happened.

src/hotspot/share/services/diagnosticCommand.hpp, line 878:
"Starts up the Java debugging if enabled in the jdwp agentlib via the onjcmd=y option."
A better wording would be:
"Starts up the Java debugging if the jdwp agentlib was enabled with the option onjcmd=y."

src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:
line 876:
replace "debug triggered by jcmd?" with " start debug via jcmd" (or "start debug on request" if we opt to change the option name from onjcmd to something more general)
line 1300:
         suspendOnInit = JNI_FALSE;
Is it really necessary/desired to set suspendOnInit to false? We could still honor suspend=y|n. The suspension will of course happen at the time debug activation is triggered.

Another question: Do we need a CSR for this?

Best regards
Christoph

> -----Original Message-----
> From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net> On
> Behalf Of Schmelter, Ralf
> Sent: Donnerstag, 6. Dezember 2018 15:54
> To: Chris Plummer <chris.plummer at oracle.com>; serviceability-
> dev at openjdk.java.net
> Subject: [CAUTION] RE: RFR (M) 8214892: Delayed starting of debugging via
> jcmd
> 
> Hi Chris,
> 
> > I think I prefer passing EI_VM_INIT for triggering_ei, but if you prefer
> > to keep it as is, I suggest a comment to clarify why it might be out of
> > range.
> 
> Actually, I used EI_VM_INIT for the longest time and only changed it
> recently, because I thought that code could assume that e.g. no classes have
> been loaded yet when getting the INIT_VM event. But since the JVMTI spec
> does not guarantees this in any way (it allows other events to be send before
> a VM_INIT), I just will change it back to use EI_VM_INIT for the initialize call.
> 
> Regarding the name of the option, I agree that onjcmd, while not technically
> fully accurate, makes most sense for the common user.
> 
> > ... It think you could just return the error right away and remove
> > the error checking code that comes later.
> 
> I've changed the code to just return the error directly.
> 
> > It's not clear to me why you want the name and address of the first
> > transport. Why is the first one necessarily the one you want?
> 
> Since currently the bag of transports must always have a size of 1, getting the
> first or the last transport is the same. But the callback function used to iterate
> the bag has to return a boolean value. I've decided to return JNI_FALSE,
> which would mean the iteration stops at the first entry.
> 
> 
> I've updated the webrev with your and Goetz Lindenmeier's suggestions:
> http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.02/
> 
> Best regards,
> Ralf