Hi, pushed: http://hg.openjdk.java.net/jdk/jdk/rev/21dfea980e23
Best regards Christoph > -----Original Message----- > From: Langer, Christoph > Sent: Dienstag, 11. Dezember 2018 11:28 > To: Schmelter, Ralf <ralf.schmel...@sap.com>; Chris Plummer > <chris.plum...@oracle.com>; serviceability-dev@openjdk.java.net > Cc: Lindenmaier, Goetz <goetz.lindenma...@sap.com> > Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd > > 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