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

[Schmelter, Ralf]

Hi All,

Please review the fix for the bug https://bugs.openjdk.java.net/browse/JDK-8214892. The webref is at http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.01/ .

This change allows debugging to be started delayed, triggered by a jcmd name VM.start_java_debugging. It works more or less like the onthrow and onuncaught options, but instead of triggering by exception we trigger it via the jcmd. This is useful when we want to trigger debugging when we see errors not related to exceptions (e.g. if a server becomes unresponsive it could be taken out of the routing and debugging is triggered to find the cause).

Best regards,
Ralf

[Chris Plummer]

Hi Ralf,

Overall looks good, but I do have a few suggestions and questions:

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.

The help for "oncmd" says "debug triggered by jcmd?". I think your use
of "cmd" should be more consistent. It's a dcmd providing the service,
and jcmd is just one way to get at the dcmd. I'm not sure of the best
way to convey this simply in the option name and help, but it should be
consistent. "ondcmd" is probably most accurate, but "onjcmd" is probably
what is most meaningful to users. I don't think it makes sense to use
"oncmd" if the help is referring to jcmd.

Your error handling in debugInit_startDebuggingViaCommand() is a bit
inconsistent. For !allowStartViaCmd you immediately return the error.
For !allowStartViaCmd you set the error, and then need to do error
checks later before eventually returning the error. It think you could
just return the error right away and remove the error checking code that
comes later.

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?

1403         bagEnumerateOver(transports, getFirstTransport, &spec);
1404
1405         if ((spec != NULL) && (transport_name != NULL) && (address
!= NULL)) {
1406             *transport_name = spec->name;
1407             *address = spec->address;
1408         }

thanks,

Chris

Post by Schmelter, Ralf
Hi All,
Please review the fix for the bug https://bugs.openjdk.java.net/browse/JDK-8214892. The webref is at http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.01/ .
This change allows debugging to be started delayed, triggered by a jcmd name VM.start_java_debugging. It works more or less like the onthrow and onuncaught options, but instead of triggering by exception we trigger it via the jcmd. This is useful when we want to trigger debugging when we see errors not related to exceptions (e.g. if a server becomes unresponsive it could be taken out of the routing and debugging is triggered to find the cause).
Best regards,
Ralf

[Schmelter, Ralf]

Hi Chris,

Post by Chris Plummer
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.

Post by Chris Plummer
... 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.

Post by Chris Plummer
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/sc

[Chris Plummer]

Hi Ralf,

I don't think the comments from Goetz made it to the list. However, with
the current changes it all looks good to me, except there are some
indentation issues. Indent tab stops are should be increments of 2 for
hotspot code. You have a few cases of using 4 in the dcmd files. For the
jdwp agent, pre-existing code uses tab stops of 4, so should remain that
way, and it looks like you've correctly done this.

thanks,

Chris

Post by Schmelter, Ralf
Hi Chris,

Post by Chris Plummer
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.

Post by Chris Plummer
... 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.

Post by Chris Plummer
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

[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-----
Behalf Of Schmelter, Ralf
Sent: Donnerstag, 6. Dezember 2018 15:54
Subject: [CAUTION] RE: RFR (M) 8214892: Delayed starting of debugging via
jcmd
Hi Chris,

Post by Chris Plummer
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.

Post by Chris Plummer
... 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.

Post by Chris Plummer
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.
http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.02/
Bes

[Schmelter, Ralf]

Hi Christoph,

Post by Langer, 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.

Post by Langer, Christoph
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.

Post by Langer, Christoph
"Starts up the Java debugging if the jdwp agentlib was enabled with the option onjcmd=y."

OK.

Post by Langer, Christoph
replace "debug triggered by jcmd?" with " start debug via jcmd"

Ok.

Post by Langer, Christoph
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 <***@sap.com>; Chris Plummer <***@oracle.com>; serviceability-***@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

Post by Langer, Christoph
-----Original Message-----
Behalf Of Schmelter, Ralf
Sent: Donnerstag, 6. Dezember 2018 15:54
Subject: [CAUTION] RE: RFR (M) 8214892: Delayed starting of debugging via
jcmd
Hi Chris,

Post by Chris Plummer
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.

Post by Chris Plummer
... 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.

Post by Chris Plummer
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.
http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.02/
Best r