RFR(MS): 8222072: JVMTI GenerateEvents() sends CompiledMethodLoad events to wrong jvmtiEnv

[serguei.spitsyn at oracle.com]

Hi Alex,

Thank you for review!

On 4/9/19 4:57 PM, Alex Menkov wrote:
> Hi Serguei,
>
> Maybe resolve code duplication in post_compiled_method_load(nmethod 
> *nm) and post_compiled_method_load(JvmtiEnv* env, nmethod *nm)?
>
> Looks like the only difference is logging, but I don't think it's 
> important as old post_compiled_method_load(JvmtiEnv* env, const 
> jmethodID method, const jint length, ...) was not used.

Another difference is that in case of GenerateEvents the phase is 
guaranteed to be LIVE.
So, there is no need for the PRINORDIAL phase check.

Also, I'm not sure yet how important the difference in logging is.
Let me think a little bit more on this.

Thanks,
Serguei

> --alex
>
> On 04/09/2019 13:58, serguei.spitsyn at oracle.com wrote:
>> Hi Jc,
>>
>> Thank you a lot for looking at this!
>>
>> On 4/9/19 9:29 AM, Jean Christophe Beyler wrote:
>>> Hi Serguei,
>>>
>>> I saw a nit here:
>>>
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.1/test/hotspot/jtreg/serviceability/jvmti/GenerateEvents/MyPackage/GenerateEventsTest.java.html 
>>> <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.1/test/hotspot/jtreg/serviceability/jvmti/GenerateEvents/MyPackage/GenerateEventsTest.java.html> 
>>>
>>>
>>> arei -> are
>>
>> Nice catch, fixed.
>>
>>> - I'm not sure you needed two files for the agents, you could have 
>>> re-used some of the code and just called twice GetEnv but that is a 
>>> detail.
>>
>> Right.
>> I initially wanted to use this way but then decided to use two real 
>> agent libraries.
>> Wanted to exercise this path.
>>
>>> - I thought JNIEnv was not supposed to be really kept because it 
>>> should not be used by another thread, is there not a risk that you 
>>> are doing that? It doesn't look like it but I've been surprised in 
>>> the past
>>
>> You are right.
>> Tried to fix it in new webrev version below.
>>
>>
>>> - Isn't there a chance that your second agent gets a normal 
>>> JVMTI_EVENT_COMPILED_METHOD_LOAD before the GenerateEvents call and 
>>> increments its counter?
>>>    - I guess we'd see if it becomes flaky at some point? :)
>> Yes, it is expected.
>> But they should be posted on threads other than Main thread.
>> The callback should ignore them.
>>
>>
>> The updated fix version is:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.2/ 
>>
>>
>>
>> Thanks!
>> Serguei
>>
>>>
>>> Thanks!
>>> Jc
>>>
>>>
>>> On Mon, Apr 8, 2019 at 12:29 PM serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com 
>>> <mailto:serguei.spitsyn at oracle.com>> wrote:
>>>
>>>     Please, review a fix for:
>>>     https://bugs.openjdk.java.net/browse/JDK-8222072
>>>
>>>     Webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.1/
>>> <http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.1/>
>>>
>>>
>>>     Summary:
>>>       The JVMTI GenerateEvents() must send CompiledMethodLoad events
>>>     only to the agent which called it.
>>>       However, it sends events to all agents (jvmti environements)
>>>     which violates the JVMTI spec.
>>>       The webrev above fixes this issue and adds new jtreg test:
>>>         test/hotspot/jtreg/serviceability/jvmti/GenerateEvents
>>>
>>>
>>>     Testing:
>>>       Mach5 submission for:
>>>        - JVMTI tests: open/test/hotspot/jtreg/vmTestbase/nsk/jvmti
>>>     - new test: test/hotspot/jtreg/serviceability/jvmti/GenerateEvents
>>>
>>>     Thanks,
>>>     Serguei
>>>
>>>
>>>
>>> -- 
>>>
>>> Thanks,
>>> Jc
>>