Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Coleen Phillimore]

We didn't file any bugs because I don't remember finding anything 
specific, other than gosh that code is scary and I wish we didn't 
have to do this.


If you find a null 'm' below and call m-print() is the method obsolete?

Coleen

On 4/30/14, 8:24 PM, Jeremy Manson wrote:

Did the new bugs get filed for this?

I'm triggering this check with a redefined class (from the 
bootclasspath, if that matters).  To investigate it a little, I 
instrumented StackTraceElement::create thus:


oop java_lang_StackTraceElement::create(methodHandle method, int bci, 
TRAPS) {

  Handle mirror (THREAD, method-method_holder()-java_mirror());
  int method_id = method-method_idnum();

  InstanceKlass* holder = method-method_holder();
  Method* m = holder-method_with_idnum(method_id);
  Method* mp = holder-find_method(method-name(), method-signature());
  method-print_name();
  fprintf(stderr,  method = %p id = %d method' = %p \n, m, 
method_id, mp);
  return create(mirror, method_id, method-constants()-version(), 
bci, THREAD);

}

m is null, and mp isn't.  method-print_name works fine.  This makes 
me feel that the idnum array is out of date somehow.  Before I go down 
the rabbit hole and try to figure out why that is, does someone else 
know why?


Thanks!

Jeremy



On Thu, Oct 3, 2013 at 11:02 AM, Coleen Phillimore 
coleen.phillim...@oracle.com mailto:coleen.phillim...@oracle.com 
wrote:


Summary: Redefined class in stack trace may not be found by
method_idnum so handle null.

This is a simple change.  I had another change to save the method
name (as u2) in the backtrace, but it's not worth the extra
footprint in backtraces for this rare case.

The root problem was that we save method_idnum in the backtrace
(u2) instead of Method* to avoid Method* from being redefined and
deallocated.  I made a change to
InstanceKlass::method_from_idnum() to return null rather than the
last method in the list, which causes this crash. Dan and I went
down the long rabbit-hole of why method_idnum is changed for
obsolete methods and we think there's some cleanup and potential
bugs in this area.  But this is not that change.  I'll file
another bug to continue this investigation for jdk9 (or 8uN).

Staffan created a test - am including core-libs for the review
request.  Also tested with all of the vm testbase tests, mlvm
tests, and java/lang/instrument tests.

open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
http://cr.openjdk.java.net/%7Ecoleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk
http://cr.openjdk.java.net/%7Ecoleenp/8025238_jdk

Thanks,
Coleen

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Staffan Larsen]

On 4 okt 2013, at 00:49, Coleen Phillmore coleen.phillim...@oracle.com wrote:

 
 Thanks Dan -
 
 On 10/3/2013 4:28 PM, Daniel D. Daugherty wrote:
  open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk
 
 test/java/lang/instrument/RedefineMethodInBacktrace.sh
No comments.
 
 test/java/lang/instrument/RedefineMethodInBacktraceApp.java
line 78: t.setDaemon(true);
Why make the target thread a daemon? Wouldn't it be a more
complete test to do Thread.join() of that thread just to
be sure that the target thread has finished (and is not
stuck)?
 
 Staffan, can you answer this?

You could do it either way. I don't have a strong opinion. It's possible that 
the deamonization is a leftover from one of my iterations of the test.

/Staffan


 
 Coleen
 
 
 test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java
 test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java
Nice sync logic with the test driver.
 
 Thumbs up.
 
 Dan
 
 
 On 10/3/13 12:02 PM, Coleen Phillimore wrote:
 Summary: Redefined class in stack trace may not be found by method_idnum so 
 handle null.
 
 This is a simple change.  I had another change to save the method name (as 
 u2) in the backtrace, but it's not worth the extra footprint in backtraces 
 for this rare case.
 
 The root problem was that we save method_idnum in the backtrace (u2) 
 instead of Method* to avoid Method* from being redefined and deallocated.  
 I made a change to InstanceKlass::method_from_idnum() to return null rather 
 than the last method in the list, which causes this crash.   Dan and I went 
 down the long rabbit-hole of why method_idnum is changed for obsolete 
 methods and we think there's some cleanup and potential bugs in this area.  
 But this is not that change.  I'll file another bug to continue this 
 investigation for jdk9 (or 8uN).
 
 Staffan created a test - am including core-libs for the review request.  
 Also tested with all of the vm testbase tests, mlvm tests, and 
 java/lang/instrument tests.
 
 open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
 bug link https://bugs.openjdk.java.net/browse/JDK-8025238
 
 test case for jdk8 repository:
 
 open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk
 
 Thanks,
 Coleen
 
 
 
 

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[serguei.spit...@oracle.com]

Coleen,

The fix looks good.
It is nice you've come up with this.

Thanks,
Serguei

On 10/3/13 11:02 AM, Coleen Phillimore wrote:
Summary: Redefined class in stack trace may not be found by 
method_idnum so handle null.


This is a simple change.  I had another change to save the method name 
(as u2) in the backtrace, but it's not worth the extra footprint in 
backtraces for this rare case.


The root problem was that we save method_idnum in the backtrace (u2) 
instead of Method* to avoid Method* from being redefined and 
deallocated.  I made a change to InstanceKlass::method_from_idnum() to 
return null rather than the last method in the list, which causes this 
crash.   Dan and I went down the long rabbit-hole of why method_idnum 
is changed for obsolete methods and we think there's some cleanup and 
potential bugs in this area.  But this is not that change.  I'll file 
another bug to continue this investigation for jdk9 (or 8uN).


Staffan created a test - am including core-libs for the review 
request.  Also tested with all of the vm testbase tests, mlvm tests, 
and java/lang/instrument tests.


open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

Thanks,
Coleen

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Daniel D. Daugherty]

 http://cr.openjdk.java.net/~coleenp/8025238/

src/share/vm/classfile/javaClasses.cpp
1804   if (method == NULL) {
1805 // leave name and fileName null
1806 java_lang_StackTraceElement::set_lineNumber(element(), -1);
Is it possible to set the name and fileName to something?
A caller may not be expecting those to be NULL.

Also, holder-method_with_idnum(method_id) should be able to
search the previous class version list and find the obsolete
Method* that matches the 'method_id' value.

src/share/vm/prims/jvmtiRedefineClasses.cpp
Better comment than the original.

Dan


On 10/3/13 12:02 PM, Coleen Phillimore wrote:
Summary: Redefined class in stack trace may not be found by 
method_idnum so handle null.


This is a simple change.  I had another change to save the method name 
(as u2) in the backtrace, but it's not worth the extra footprint in 
backtraces for this rare case.


The root problem was that we save method_idnum in the backtrace (u2) 
instead of Method* to avoid Method* from being redefined and 
deallocated.  I made a change to InstanceKlass::method_from_idnum() to 
return null rather than the last method in the list, which causes this 
crash.   Dan and I went down the long rabbit-hole of why method_idnum 
is changed for obsolete methods and we think there's some cleanup and 
potential bugs in this area.  But this is not that change.  I'll file 
another bug to continue this investigation for jdk9 (or 8uN).


Staffan created a test - am including core-libs for the review 
request.  Also tested with all of the vm testbase tests, mlvm tests, 
and java/lang/instrument tests.


open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

Thanks,
Coleen

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Coleen Phillmore]

Thanks Dan -

On 10/3/2013 4:28 PM, Daniel D. Daugherty wrote:

 open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

test/java/lang/instrument/RedefineMethodInBacktrace.sh
No comments.

test/java/lang/instrument/RedefineMethodInBacktraceApp.java
line 78: t.setDaemon(true);
Why make the target thread a daemon? Wouldn't it be a more
complete test to do Thread.join() of that thread just to
be sure that the target thread has finished (and is not
stuck)?


Staffan, can you answer this?

Coleen



test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java
test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java
Nice sync logic with the test driver.

Thumbs up.

Dan


On 10/3/13 12:02 PM, Coleen Phillimore wrote:
Summary: Redefined class in stack trace may not be found by 
method_idnum so handle null.


This is a simple change.  I had another change to save the method 
name (as u2) in the backtrace, but it's not worth the extra footprint 
in backtraces for this rare case.


The root problem was that we save method_idnum in the backtrace (u2) 
instead of Method* to avoid Method* from being redefined and 
deallocated.  I made a change to InstanceKlass::method_from_idnum() 
to return null rather than the last method in the list, which causes 
this crash.   Dan and I went down the long rabbit-hole of why 
method_idnum is changed for obsolete methods and we think there's 
some cleanup and potential bugs in this area.  But this is not that 
change.  I'll file another bug to continue this investigation for 
jdk9 (or 8uN).


Staffan created a test - am including core-libs for the review 
request.  Also tested with all of the vm testbase tests, mlvm tests, 
and java/lang/instrument tests.


open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

Thanks,
Coleen

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Coleen Phillmore]

Thanks Serguei!
Coleen
On 10/3/2013 3:50 PM, serguei.spit...@oracle.com wrote:

Coleen,

The fix looks good.
It is nice you've come up with this.

Thanks,
Serguei

On 10/3/13 11:02 AM, Coleen Phillimore wrote:
Summary: Redefined class in stack trace may not be found by 
method_idnum so handle null.


This is a simple change.  I had another change to save the method 
name (as u2) in the backtrace, but it's not worth the extra footprint 
in backtraces for this rare case.


The root problem was that we save method_idnum in the backtrace (u2) 
instead of Method* to avoid Method* from being redefined and 
deallocated.  I made a change to InstanceKlass::method_from_idnum() 
to return null rather than the last method in the list, which causes 
this crash.   Dan and I went down the long rabbit-hole of why 
method_idnum is changed for obsolete methods and we think there's 
some cleanup and potential bugs in this area.  But this is not that 
change.  I'll file another bug to continue this investigation for 
jdk9 (or 8uN).


Staffan created a test - am including core-libs for the review 
request.  Also tested with all of the vm testbase tests, mlvm tests, 
and java/lang/instrument tests.


open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

Thanks,
Coleen

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Coleen Phillmore]

On 10/3/2013 7:01 PM, Daniel D. Daugherty wrote:

On 10/3/13 4:56 PM, Coleen Phillmore wrote:


Thanks Dan -

On 10/3/2013 4:07 PM, Daniel D. Daugherty wrote:

 http://cr.openjdk.java.net/~coleenp/8025238/

src/share/vm/classfile/javaClasses.cpp
1804   if (method == NULL) {
1805 // leave name and fileName null
1806 java_lang_StackTraceElement::set_lineNumber(element(), -1);
Is it possible to set the name and fileName to something?
A caller may not be expecting those to be NULL.

Also, holder-method_with_idnum(method_id) should be able to
search the previous class version list and find the obsolete
Method* that matches the 'method_id' value.



We don't save the obsolete versions on the previous version list, 
only the emcp versions.  I just looked at the old code and that's 
always been the case.   So the method that has the method_idnum that 
isn't supposed to be found is an obsolete method.


Clearly I've forgotten... thumbs up!


:)
Thanks,
Coleen



Dan




Coleen


src/share/vm/prims/jvmtiRedefineClasses.cpp
Better comment than the original.

Dan


On 10/3/13 12:02 PM, Coleen Phillimore wrote:
Summary: Redefined class in stack trace may not be found by 
method_idnum so handle null.


This is a simple change.  I had another change to save the method 
name (as u2) in the backtrace, but it's not worth the extra 
footprint in backtraces for this rare case.


The root problem was that we save method_idnum in the backtrace 
(u2) instead of Method* to avoid Method* from being redefined and 
deallocated.  I made a change to InstanceKlass::method_from_idnum() 
to return null rather than the last method in the list, which 
causes this crash. Dan and I went down the long rabbit-hole of why 
method_idnum is changed for obsolete methods and we think there's 
some cleanup and potential bugs in this area.  But this is not that 
change.  I'll file another bug to continue this investigation for 
jdk9 (or 8uN).


Staffan created a test - am including core-libs for the review 
request.  Also tested with all of the vm testbase tests, mlvm 
tests, and java/lang/instrument tests.


open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk

Thanks,
Coleen