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

[serguei.spitsyn at oracle.com]

Hi Alex,

New webrev version with refactoring:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222072-jvmti-GenerateEvents.3/

I will also re-submit mach5 jobs for jvmti tests and new test.

Thanks,
Serguei


On 4/9/19 5:39 PM, serguei.spitsyn at oracle.com wrote:
> 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
>>>
>