On 11/15/19 2:13 PM, Chris Plummer wrote:
On 11/15/19 4:39 AM, [email protected] wrote:
On 11/14/19 9:07 PM, Chris Plummer wrote:
Hi Coleen,
Is it ok to end up missing some CompiledMethodLoad events? The spec
says:
"Sent when a method is compiled and loaded into memory by the VM. If
it is unloaded, the CompiledMethodUnload event is sent. If it is
moved, the CompiledMethodUnload event is sent, followed by a new
CompiledMethodLoad event. Note that a single method may have
multiple compiled forms, and that this event will be sent for each
form. "
So a method was still "compiled and loaded into memory", right? We
just didn't get the event out before it was too late. Is the
CompiledMethodUnload still sent?
Yes, the CompiledMethodUnload event would be sent for this.
My first version of my change reported the event without the extra
information (inlining and some code blob address location maps).
Maybe that would be better. Here it is and tested successfully with
the testcase that crashed.
open webrev at
http://cr.openjdk.java.net/~coleenp/2019/8173361.02/webrev
The more I look at this code, the more problems I see.
get_and_cache_jmethod_id() will send back a jmethodID from when the
method was live. But like the class unloading event, a user cannot
trust that the Method* in the jmethodID points to anything valid.
So the spec for CompiledMethodLoad event should say for the method,
like the CompiledMethodUnload event:
Compiled method being unloaded. For identification of the compiled
method only -- the class may be unloaded and therefore the method
should not be used as an argument to further JNI or JVMTI functions.
Yes, I agree that with this approach a spec clarification is needed. I
just wonder about compatibility for agents that assume it is a valid
jmethodID. I suppose if an agent treated it as valid and crashed as a
result, this is just moving the crash from the jvmti impl to the
agent. We also have to consider that agents might currently treat it
as valid for functional purposes, and likely never run into this
problem, but with the spec update technically that would mean that
they would no longer be able to. However, what's likely is that any
existing agent would just continue to be ignorant of this spec change,
and continue to run with no issue.
I'm not sure I'm following this. Yes, if an agent tries to do something
with the jmethodID, like call it, in the case where it has been
unloaded, it will crash.
The nmethod was unloaded because some oop in it was no longer valid. It
may be that the class is unloaded, in which case the Method* in the
jmethodID is a bad pointer. So that was a preexisting problem.
The CompiledMethodLoad event saves the jmethodID from when the nmethod
was created, so it's good at this point. If the nmethod is unloaded
because some *other* oop in it is bad, the Method* would still be
valid. My change retrieves the one saved when the load event was
created. It's only the case if the nmethod is unloaded because the
class containing the Method* in _method is unloaded that would get a crash.
The specification should still be clarified to say this though.
Or we don't send the event like 01. Either one doesn't crash.
Yeah, I guess I don't have a good answer here. Seems like both
approaches have issues. Maybe the correct fix is to keep the nm live
until the deferred event can be sent.
This is not feasable, since the nmethod may have bad oops in it.
Coleen
thanks,
Chris
Thanks,
Coleen
thanks,
Chris
On 11/14/19 5:15 PM, [email protected] wrote:
Summary: Don't post information which uses metadata from unloaded
nmethods
Tested tier1-3 and 100 times with test that failed (reproduced
failure without the fix).
open webrev at
http://cr.openjdk.java.net/~coleenp/2019/8173361.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8173361
Thanks,
Coleen
