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

[Alex Menkov]

Much simpler and cleaner!
LGTM.

--alex

On 04/09/2019 18:56, serguei.spitsyn at oracle.com wrote:
> 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
>>>>
>>
>