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

[Jeremy Manson]

Finally had a chance to look at this again.  The method is not marked
obsolete.

I can't refactor the code so that the full version of create takes the
Method *, because it is called to resolve stack traces from Throwables, and
there is no way to map back to Method * from the method_ids that are stored
in Throwables.

So, my workaround (below) works fine to deal with getAllStackTraces, but
doesn't work if there is an obsolete method in  a Throwable's stack trace.
 I can't reproduce that, but that doesn't mean it won't happen.

The only thing I can think to do about this is to keep track of obsolete
method_ids.  For example, when we do a retransform, InstanceKlass.methods
could keep the old method around.  Or, Methods could keep track of previous
method ids (or a subclass of Method could keep track of previous method
ids, so that most Methods don't suffer from bloat issues keeping track of
something that is rarely / never there).

I assume that there is a reason we don't do this, though.

Here's my diff.  I think it is fair to call it a crude hack.  If there is
something else you would like me to try, I'm happy to do so.

diff --git a/src/share/vm/classfile/javaClasses.cpp
b/src/share/vm/classfile/javaClasses.cpp
--- a/src/share/vm/classfile/javaClasses.cpp
+++ b/src/share/vm/classfile/javaClasses.cpp
@@ -1857,6 +1857,14 @@
 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);
+  if (m == NULL) {
+    Method* mp = holder->find_method(method->name(), method->signature());
+    if (mp != NULL) {
+      method_id = mp->method_idnum();
+    }
+  }
   return create(mirror, method_id, method->constants()->version(), bci,
THREAD);
 }



Jeremy



On Wed, Apr 30, 2014 at 10:13 PM, Jeremy Manson <jeremymanson at google.com>wrote:

> I'll have to check tomorrow, but it's not unreasonable to think that it
> is.  It is a stack frame on a thread that was started before the class
> redefinition happened.
>
> I guess the question is: what makes sense to do here?  I put in a
> workaround such that, if m is null, method_id is replaced by the method_id
> from mp.  I could refactor the code a little, such that the full version of
> create() took the Method* instead of taking the method_id, and pass it mp
> if m is null.
>
> It'll be hard to get you a reproducible test case, though.  This only
> happens in an integration test in our test cluster.  Debugging was painful.
>
> Jeremy
>
>
> On Wed, Apr 30, 2014 at 6:34 PM, Coleen Phillimore <
> coleen.phillimore at oracle.com> wrote:
>
>>
>> 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.phillimore at 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/
>>> 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
>>>
>>
>>
>>
>