RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Alan, Ralf has opened a CSR for JDK-8214892: https://bugs.openjdk.java.net/browse/JDK-8223456 What will we do now? Shall we use this CSR to discuss how the VM options and jcmds/dcmds for "debugging on demand" will be named and what they will be? And in case, the outcome is different to what'll be actually implemented, will we do another change, referring to that CSR? I think I have some suggestions about the option(s). Shall I post comments to the CSR for that? Thanks Christoph > -Original Message- > From: Alan Bateman > Sent: Dienstag, 7. Mai 2019 15:25 > To: Schmelter, Ralf ; Langer, Christoph > ; serviceability-dev@openjdk.java.net > Subject: Re: RFR 8223065: Add jcmd to get the listen address of the debugger > > On 07/05/2019 14:13, Schmelter, Ralf wrote: > > Hi Christoph, > > > > thanks for the review. I've updated the webrev: > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.4/ > > > Specifying onjcmd=y to the debugger agent looks very strange. Was there > a CSR submitted for JDK-8214892? I suspect that sub-option needs further > discussion before additional features on built on it. > > -Alan
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Ralf, thanks for the new webrev. It looks good. In GetListenAddressTest.java, the directive "@modules java.base/jdk.intenal.vm" can be removed as well. I've loaded the patch with this modification into our test system. But anyway, we'll need to wait for the CSR discussion until it can be pushed. Best regards Christoph > -Original Message- > From: Schmelter, Ralf > Sent: Dienstag, 7. Mai 2019 15:13 > To: Langer, Christoph ; serviceability- > d...@openjdk.java.net > Subject: RE: RFR 8223065: Add jcmd to get the listen address of the debugger > > Hi Christoph, > > thanks for the review. I've updated the webrev: > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.4/ > > Best regards, > Ralf >
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
On 07/05/2019 14:13, Schmelter, Ralf wrote: Hi Christoph, thanks for the review. I've updated the webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.4/ Specifying onjcmd=y to the debugger agent looks very strange. Was there a CSR submitted for JDK-8214892? I suspect that sub-option needs further discussion before additional features on built on it. -Alan
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Christoph, thanks for the review. I've updated the webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.4/ Best regards, Ralf
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Ralf, the change looks good to me, overall. I found a few minor nits in the tests. test/jdk/com/sun/jdi/OnJcmdTest.java 3 * Copyright (c) 2018, 2019, SAP SE. All rights reserved. -> according to our guidelines, we should not have a ',' after the last year for the SAP copyrights 33 * @run compile --add-exports java.base/jdk.internal.vm=ALL-UNNAMED -g OnJcmdTest.java -> can you replace this with @modules java.base/jdk.internal.vm ? The -g option is also not required, I guess (unless somebody debugs the test) Then you should be able to change 34 * @run main/othervm --add-exports java.base/jdk.internal.vm=ALL-UNNAMED -agentlib:jdwp=transport=dt_socket,address=localhost:0,onjcmd=y,server=y OnJcmdTest into @run main/othervm -agentlib:jdwp=transport=dt_socket,address=localhost:0,onjcmd=y,server=y OnJcmdTest 37 import java.lang.reflect.Method; -> can be removed test/jdk/com/sun/jdi/GetListenAddressTest.java -> SAP copyright header needs fixing like above -> 33 * @run compile -g GetListenAddressTest.java should be removed -> 37 import java.lang.ProcessBuilder.Redirect; -> is unnecessary, should be removed I'll sponsor the change for you. Best regards Christoph > -Original Message- > From: serviceability-dev On > Behalf Of Baesken, Matthias > Sent: Montag, 6. Mai 2019 12:55 > To: serviceability-dev@openjdk.java.net > Subject: [CAUTION] Re: RFR 8223065: Add jcmd to get the listen address of > the debugger > > Hello, looked at the latest web rev ( > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.3/ ) , > looks good to me ! > (not a Reviewer however ) > > > Best regards, Matthias > > > > > > On 30/04/2019 9:33 pm, Schmelter, Ralf wrote: > > > Hi David, > > > > > > good catch. I've moved the vm->native transition back to the start of the > > method and instead do a native->vm transition before calling > > print_debug_listen_address() method. > > > > > > webrev: > > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ > > > > Yep that works too. :) > > > > Not a review as I didn't look at the rest of the code. > > > > Cheers, > > David > > > > > Best regards, > > > Ralf > > > > > > >
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
Hello, looked at the latest web rev ( http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.3/ ) , looks good to me ! (not a Reviewer however ) Best regards, Matthias > > On 30/04/2019 9:33 pm, Schmelter, Ralf wrote: > > Hi David, > > > > good catch. I've moved the vm->native transition back to the start of the > method and instead do a native->vm transition before calling > print_debug_listen_address() method. > > > > webrev: > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ > > Yep that works too. :) > > Not a review as I didn't look at the rest of the code. > > Cheers, > David > > > Best regards, > > Ralf > > > >
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
On 4/30/19 4:30 AM, Schmelter, Ralf wrote: Hi Chris. I think print_debug_listen_address() should have some exception checking added after the java calls. That is already done by the CHECK_false macro. It checks if an exception is occurred, and returns with false if this is the case. And depending on how the dcmd was called, this exception is then handled (e.g. if called via jcmd, the exception is printed and if called the jmm_ExecuteDiagnosticCommand method, the exception is delivered to the Java code). Ok. I'm a little unsure why you modified DebugOnCmdStartDCmd to use print_debug_listen_address(), but still have a fallback to print the specified transport and address. Yeah, the fallback is really not needed, since the debugger is not listening in this case. I've removed it from the code. If anything I would have written a get_debug_listen_address() and used it to verify that the specified and actual addresses end up being the same (and then also make print_debug_listen_address() use this API). Because the requested address and the actual address the debugger is listening can be different. In the case of sockets, if you request localhost:0, the system will find an unused port, so the listening address will be the actual port on which we listen. Ok. You need to update copyright date to 2019. Fixed. Can you write a test for this new dcmd. You can probably just extend OnJcmdTest.java. The GetListenAddressTest tests the new dcmd. Ok. I missed it because when after reviewing the OnJcmdTest.java changes in "frames" mode and then clicking on "next" to review the next file, it returns you to the index. Seems this is normal behavior for new files in webrevs since there is no "frames" mode for them. Changes look good. Chris I've updated the webrev (including David's suggestion): http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ Best regards, Ralf
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Ralf, On 30/04/2019 9:33 pm, Schmelter, Ralf wrote: Hi David, good catch. I've moved the vm->native transition back to the start of the method and instead do a native->vm transition before calling print_debug_listen_address() method. webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ Yep that works too. :) Not a review as I didn't look at the rest of the code. Cheers, David Best regards, Ralf
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi David, good catch. I've moved the vm->native transition back to the start of the method and instead do a native->vm transition before calling print_debug_listen_address() method. webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ Best regards, Ralf
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Chris. > I think print_debug_listen_address() should have some exception checking > added after the java calls. That is already done by the CHECK_false macro. It checks if an exception is occurred, and returns with false if this is the case. And depending on how the dcmd was called, this exception is then handled (e.g. if called via jcmd, the exception is printed and if called the jmm_ExecuteDiagnosticCommand method, the exception is delivered to the Java code). > I'm a little unsure why you modified DebugOnCmdStartDCmd to use > print_debug_listen_address(), but still have a fallback to print the > specified transport and address. Yeah, the fallback is really not needed, since the debugger is not listening in this case. I've removed it from the code. > If anything I would have written a > get_debug_listen_address() and used it to verify that the specified and > actual addresses end up being the same (and then also make > print_debug_listen_address() use this API). Because the requested address and the actual address the debugger is listening can be different. In the case of sockets, if you request localhost:0, the system will find an unused port, so the listening address will be the actual port on which we listen. > You need to update copyright date to 2019. Fixed. > Can you write a test for this new dcmd. You can probably just extend > OnJcmdTest.java. The GetListenAddressTest tests the new dcmd. I've updated the webrev (including David's suggestion): http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ Best regards, Ralf
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
Responding to Chris's question below ... On 30/04/2019 3:58 am, Chris Plummer wrote: Hi Ralf, I think print_debug_listen_address() should have some exception checking added after the java calls. I'm a little unsure why you modified DebugOnCmdStartDCmd to use print_debug_listen_address(), but still have a fallback to print the specified transport and address. If anything I would have written a get_debug_listen_address() and used it to verify that the specified and actual addresses end up being the same (and then also make print_debug_listen_address() use this API). I'm also unsure of your ThreadToNativeFromVM change. This is not an area I understand well, so best to get someone else to ok it. I can see that the new code for print_debug_listen_address needs to be executed whilst still _thread_in_vm so the ThreadToNativeFromVM helper can't extend over that part. However, I would suggest that you otherwise keep it covering as much of the existing code as possible - specifically the chunk that calls os::find_agent_function. I would be concerned that the eventual calls to dlsym etc may potentially take some time and this would prevent safepoints from occurring as this thread is still "in VM". This might be overly conservative but it avoids any possibility of introducing a change in behaviour for the existing code. Cheers, David - You need to update copyright date to 2019. Can you write a test for this new dcmd. You can probably just extend OnJcmdTest.java. thanks, Chris On 4/29/19 8:31 AM, Schmelter, Ralf wrote: Thanks for the review. I've update the webrev to use explicit NULL checks: https://bugs.openjdk.java.net/browse/JDK-8223065 And I now use the pointer to the first byte in the result to split the property value, since I might need the calculate the pointer past the last character (if the prop ends with ':'). I cannot see the SEGV, but I've scheduled the patch to be tested in our nightly build again, so maybe I can reproduce it there. Best regards, Ralf
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Ralf, I think print_debug_listen_address() should have some exception checking added after the java calls. I'm a little unsure why you modified DebugOnCmdStartDCmd to use print_debug_listen_address(), but still have a fallback to print the specified transport and address. If anything I would have written a get_debug_listen_address() and used it to verify that the specified and actual addresses end up being the same (and then also make print_debug_listen_address() use this API). I'm also unsure of your ThreadToNativeFromVM change. This is not an area I understand well, so best to get someone else to ok it. You need to update copyright date to 2019. Can you write a test for this new dcmd. You can probably just extend OnJcmdTest.java. thanks, Chris On 4/29/19 8:31 AM, Schmelter, Ralf wrote: Thanks for the review. I've update the webrev to use explicit NULL checks: https://bugs.openjdk.java.net/browse/JDK-8223065 And I now use the pointer to the first byte in the result to split the property value, since I might need the calculate the pointer past the last character (if the prop ends with ':'). I cannot see the SEGV, but I've scheduled the patch to be tested in our nightly build again, so maybe I can reproduce it there. Best regards, Ralf
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Thanks for the review. I've update the webrev to use explicit NULL checks: https://bugs.openjdk.java.net/browse/JDK-8223065 And I now use the pointer to the first byte in the result to split the property value, since I might need the calculate the pointer past the last character (if the prop ends with ':'). I cannot see the SEGV, but I've scheduled the patch to be tested in our nightly build again, so maybe I can reproduce it there. Best regards, Ralf
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
In diagnosticCommand.cpp you'll want to use "res != 0"
on lines 1084 and 1086 to avoid compiler warnings
about ambiguous conversions to boolean.
1082 // The result should be a byte array or null
1083 typeArrayOop res = (typeArrayOop) result.get_jobject();
1084 assert(!res || (TypeArrayKlass::cast(res->klass())->element_type() == T_BYTE),
"Must be byte array");
1085
1086 if (res&& (res->length()> 0)) {
I see a SEGV in print_debug_listen_address running
GetListenAddressTest with a linux-x64-debug build.
Can you triage the bug and target the fixVersion for 13.
Thanks
On 4/29/19, 9:19 AM, Schmelter, Ralf wrote:
Please review the patch which adds a jcmd to get the actual address the
debugging backend is listening on.
The this value was stored in the agent property sun.jdwp.listenerAddress and
currently only used by the ProcessAttachingConnector.
Additionally, the listen address is now displayed by the
VM.start_java_debugging command, if a new session was started.
webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.0/
bugreport: https://bugs.openjdk.java.net/browse/JDK-8223065
Best regards,
Ralf
RFR 8223065: Add jcmd to get the listen address of the debugger
Please review the patch which adds a jcmd to get the actual address the debugging backend is listening on. The this value was stored in the agent property sun.jdwp.listenerAddress and currently only used by the ProcessAttachingConnector. Additionally, the listen address is now displayed by the VM.start_java_debugging command, if a new session was started. webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.0/ bugreport: https://bugs.openjdk.java.net/browse/JDK-8223065 Best regards, Ralf
