Hi, @Ralf: Thanks for updating the patch. I'm good with it now 😊 As there are no other opinions about the naming of the jdwp agent property, I'm fine with "onjcmd". It is the main use case.
@all: With me and Chris as reviewers, I think we are good to push. I'll do this tomorrow if I don't hear objections until then. Thanks and best regards Christoph > -----Original Message----- > From: Schmelter, Ralf > Sent: Monday, December 10, 2018 2:55 PM > To: Langer, Christoph <christoph.lan...@sap.com>; Chris Plummer > <chris.plum...@oracle.com>; serviceability-dev@openjdk.java.net > Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd > > Hi Christoph, > > > 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? > > I'm not sure making the name more generic is the right move, when it will > probably be enabled via the jcmd in >95% of the cases. > > > 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. > > OK, I've changed that. > > > A better wording would be: > > "Starts up the Java debugging if the jdwp agentlib was enabled with the > option onjcmd=y." > > OK. > > > replace "debug triggered by jcmd?" with " start debug via jcmd" > > Ok. > > > 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. > > I don't think it would make sense to support suspend=y. In makes sense > when starting the debugging directly (so no, or at least not much) Java code > can be executed and you can debug from the beginning. And it also makes > sense for the onthrow/onuncaught, since you can to see the exceptions in > the debugger. But when triggered from the outside, you have no natural > event to wait for and no state to preserve. The only effect you would see is > that the jcmd would not return, since the initialize method would be blocked > until a debugger has been connected. > > I've updated the webrev: > http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.03/ > > Best regards, > Ralf Schmelter > > > -----Original Message----- > From: Langer, Christoph > Sent: Freitag, 7. Dezember 2018 09:41 > To: Schmelter, Ralf <ralf.schmel...@sap.com>; Chris Plummer > <chris.plum...@oracle.com>; serviceability-dev@openjdk.java.net > Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd > > 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-boun...@openjdk.java.net> > On > > Behalf Of Schmelter, Ralf > > Sent: Donnerstag, 6. Dezember 2018 15:54 > > To: Chris Plummer <chris.plum...@oracle.com>; serviceability- > > d...@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
