Thanks Dan for looking at this.
On 11/15/19 11:29 AM, Daniel D. Daugherty wrote:
On 11/15/19 7:48 AM, [email protected] wrote:
I meant to add myself to the To list.
On 11/15/19 7: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
src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp
No comments.
src/hotspot/share/prims/jvmtiExport.cpp
L2132: // It's not safe to look at metadata for unloaded methods.
L2133: if (nm->method() == NULL) {
L2134: return NULL;
nit - The comment is about why we're returning NULL early so
perhaps the comment should be inside the if-statement (between
L2133 and L2134).
Sure, I can change that.
In the previous version (01) you checked (!nm->is_alive()) in a
different place. That function is defined:
bool is_alive() const { return _state < unloaded; }
And states like unloaded are defined like this:
enum { not_installed = -1, // in construction, only the owner doing
the construction is
// allowed to advance state
in_use = 0, // executable nmethod
not_used = 1, // not entrant, but revivable
not_entrant = 2, // marked for deoptimization but
activations may still exist,
// will be transformed to zombie when all
activations are gone
unloaded = 3, // there should be no activations, should
not be called, will be
// transformed to zombie by the sweeper,
when not "locked in vm".
zombie = 4 // no activations exist, nmethod is ready
for purge
};
so by switching to (nm->method() == NULL) you are going more
directly to whether there is useful data available, but it
might be racier. I'm not sure at which state nm->method()
starts to return NULL. I'm also not sure if nm->method() can
return NULL for any of the earlier states, e.g., not_used.
I agree with your earlier comment that nm->is_alive() seems
safer. Your call on which to use.
I think my earlier comment was wrong. If the method is unloaded, the
_method field is zeroed before the unloaded _state is set. With
concurrent class unloading, this event posting can run at the same time
as make_unloaded(), if I'm reading this correctly.
I should actually use Atomic::load to get the _method, in both of the
places where I get the field.
The _method is also zeroed with make_zombie call but this is blocked out
by the nmethodLocker.
Thanks,
Coleen
Thumbs up. My only non-theory comment is a nit so I don't need to
see a new webrev.
Dan
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.
Or we don't send the event like 01. Either one doesn't crash.
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
