Re: RFR (M) 8223044: Add back exception checking in tests

2019-04-29 Thread Chris Plummer 

  
  
Hi JC,



On 4/29/19 4:25 PM, Jean Christophe
  Beyler wrote:


  
  Hi Chris,


I agree it's not ideal, how about putting it first?
  
  73 loadedClass = (jclass)
jni_env->CallObjectMethod(TRACE_JNI_CALL, loader,
methodID, className);
  

  

It's not consistent with other uses, or are you suggesting changing
them all?

  


I find that less awkward than the VAR_ARGS, no?


Or something like:

  
 73 loadedClass = (jclass)
  jni_env->CallObjectMethodTraced(loader, methodID,
  className);

  
  
  
  Where CallObjectMethodTraced is actually a define in the
exception handling system that adds the line and filename...

  

I don't like macroizing a method call in this manner (macros should
be all upper case, right?). Also it's inconsistent with the other
tracing calls, unless you plan on doing it to all of them.

  

  
  
  Which looks better?

  

Not sure. I wouldn't ask which is better. I'd ask which is less
  offensive. :)
thanks,
Chris


  

  
  
  Thanks again,
  Jc

  
  
  
On Mon, Apr 29, 2019 at 3:40
  PM Chris Plummer 
  wrote:


  
Ok.
  I only half heatedly suggest the following



  
  73 loadedClass = (jclass)
  jni_env->CallObjectMethod(loader, methodID,
  VAR_ARGS(className));


And
  then VAR_ARGS can contain the reference to TRACE_JNI_CALL.


Chris


On
  4/29/19 2:58 PM, Jean Christophe Beyler wrote:


  Hi Chris,


Thanks for the review! It is the only issue I have
  with the system now and I had not found a good
  solution for:


as CallObjectMethod is a variadic method, I can't
  put TRACE_JNI_CALL at the end; so I put right before
  the end; that allows me to do this:

  
+jobject ExceptionCheckingJniEnv::CallObjectMethod(jobject obj, jmethodID methodID, int line,
+ const char* file_name, ...) {
+  JNIVerifier<> marker(this, "CallObjectMethod", obj, methodID, line, file_name);
+
+  va_list args;
+  va_start(args, file_name);
+  jobject result = _jni_env->CallObjectMethodV(obj, methodID, args);
+  va_end(args);
+  return result;
+}
  Putting it at the end would mean I would have to play with the va_list to remove the last one, and from what I have understood with va_list, it's kind of a no-no to do so. So I left at this.
  Do you have any better suggestion?

  Thanks!
  Jc

  
  
  
On Mon, Apr 29, 2019
  at 10:38 AM Chris Plummer 
  wrote:


  
Hi
  JC,
  
  In em01t002.cpp, is this correct?
  
    73 loadedClass = (jclass)
  jni_env->CallObjectMethod(loader, methodID,
  TRACE_JNI_CALL, className);
  
  Shouldn't the TRACE_JNI_CALL arg be last? If so,
  can you look into why this test didn't fail as a
  result.
  
  Other than that the changes look good.
  
  thanks,
  
  Chris
  
  On 4/26/19 5:52 PM, Jean Christophe Beyler wrote:


  Hi all,


(Re-sending with subject line complete :-))


Since JDK-8213501 finally merged (sorry it
  took so long), I am able to continue this
  work. Here is the work that puts back the
  messages for any calls that were moved around
  due to JDK-8212884.

Re: RFR (M) 8223044: Add back exception checking in tests

2019-04-29 Thread Chris Plummer 

  
  
Ok. I only half heatedly suggest the
  following



   73 loadedClass = (jclass)
  jni_env->CallObjectMethod(loader, methodID,
  VAR_ARGS(className));


And then VAR_ARGS can contain the
  reference to TRACE_JNI_CALL.


Chris


On 4/29/19 2:58 PM, Jean Christophe
  Beyler wrote:


  
  Hi Chris,


Thanks for the review! It is the only issue I have with the
  system now and I had not found a good solution for:


as CallObjectMethod is a variadic method, I can't put
  TRACE_JNI_CALL at the end; so I put right before the end; that
  allows me to do this:

  
+jobject ExceptionCheckingJniEnv::CallObjectMethod(jobject obj, jmethodID methodID, int line,
+ const char* file_name, ...) {
+  JNIVerifier<> marker(this, "CallObjectMethod", obj, methodID, line, file_name);
+
+  va_list args;
+  va_start(args, file_name);
+  jobject result = _jni_env->CallObjectMethodV(obj, methodID, args);
+  va_end(args);
+  return result;
+}
  Putting it at the end would mean I would have to play with the va_list to remove the last one, and from what I have understood with va_list, it's kind of a no-no to do so. So I left at this.
  

  Do you have any better suggestion?

  Thanks!
  Jc

  
  
  
On Mon, Apr 29, 2019 at 10:38
  AM Chris Plummer 
  wrote:


  
Hi
  JC,
  
  In em01t002.cpp, is this correct?
  
    73 loadedClass = (jclass)
  jni_env->CallObjectMethod(loader, methodID,
  TRACE_JNI_CALL, className);
  
  Shouldn't the TRACE_JNI_CALL arg be last? If so, can you
  look into why this test didn't fail as a result.
  
  Other than that the changes look good.
  
  thanks,
  
  Chris
  
  On 4/26/19 5:52 PM, Jean Christophe Beyler wrote:


  Hi all,


(Re-sending with subject line complete :-))


Since JDK-8213501 finally merged (sorry it took so
  long), I am able to continue this work. Here is the
  work that puts back the messages for any calls that
  were moved around due to JDK-8212884.
  
  
  Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.00/
  Bug: https://bugs.openjdk.java.net/browse/JDK-8223044
  
  
  All modified tests pass on my local dev machine.
  
  
  
Thanks,
  Jc

  

  



  

  
  
  
  
  -- 
  

  
  
  Thanks,
  Jc

Re: RFR 8223065: Add jcmd to get the listen address of the debugger

2019-04-29 Thread Chris Plummer 

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 (M) 8223044: Add back exception checking in tests

2019-04-29 Thread Chris Plummer 

  
  
Hi JC,
  
  In em01t002.cpp, is this correct?
  
    73 loadedClass = (jclass)
  jni_env->CallObjectMethod(loader, methodID, TRACE_JNI_CALL,
  className);
  
  Shouldn't the TRACE_JNI_CALL arg be last? If so, can you look into
  why this test didn't fail as a result.
  
  Other than that the changes look good.
  
  thanks,
  
  Chris
  
  On 4/26/19 5:52 PM, Jean Christophe Beyler wrote:


  
  Hi all,


(Re-sending with subject line complete :-))


Since JDK-8213501 finally merged (sorry it took so long), I
  am able to continue this work. Here is the work that puts back
  the messages for any calls that were moved around due
  to JDK-8212884.
  
  
  Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.00/
  Bug: https://bugs.openjdk.java.net/browse/JDK-8223044
  
  
  All modified tests pass on my local dev machine.
  
  
  
Thanks,
  Jc

Re: RFR (M)

2019-04-29 Thread Chris Plummer 

  
  
Hi JC,
  
  In em01t002.cpp, is this correct?
  
    73 loadedClass = (jclass)
  jni_env->CallObjectMethod(loader, methodID, TRACE_JNI_CALL,
  className);
  
  Shouldn't the TRACE_JNI_CALL arg be last? If so, can you look into
  why this test didn't fail as a result.
  
  Other than that the changes look good.
  
  thanks,
  
  Chris
  
  On 4/26/19 4:19 PM, Jean Christophe Beyler wrote:


  
  

  
Hi all,
  
  
  Since JDK-8213501 finally merged (sorry it took so
long), I am able to continue this work. Here is the work
that puts back the messages for any calls that were
moved around due to JDK-8212884.


Webrev: http://cr.openjdk.java.net/~jcbeyler/8223044/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8223044


All modified tests pass on my local dev machine.



  Thanks,
Jc

RE: RFR 8223065: Add jcmd to get the listen address of the debugger

2019-04-29 Thread Schmelter, Ralf 
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

2019-04-29 Thread Gary Adams 

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

2019-04-29 Thread Schmelter, Ralf 
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