[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(XXS): 8236124: Minimal VM slowdebug build failed after JDK-8212160
From:       serguei.spitsyn () oracle ! com
Date:       2020-01-07 19:49:20
Message-ID: 4bed8383-335b-ca25-57ed-f42533f136f9 () oracle ! com
[Download RAW message or body]



On 1/7/20 11:45 AM, coleen.phillimore@oracle.com wrote:
>
> On 1/7/20 2:25 PM, serguei.spitsyn@oracle.com wrote:
>> Chris,
>>
>> The macro NOT_JVMTI_RETURN is never used outside of the prims/ folder.
>> Also, there is more work to get rid of the JVMTI code in the 
>> ServiceThread.
>> I don't know how important is this for the minimal build.
>> So, I'd leave it alone for now and just fix the build issue.
>
> I think minimal vm should to exclude code at a larger granularity than 
> this, ie. jvmtiThreadState.cpp isn't compiled at all.  We want to 
> avoid as many #ifdef INCLUDE_JVMTI as possible.   I don't think you 
> should add it to serviceThread.cpp.

Thanks, Coleen.
I've checked with Chris, and he is also okay with this.
So, I'll push the latest (2nd) patch.

Thanks,
Serguei

> thanks,
> Coleen
>
>>
>> Thanks,
>> Serguei
>>
>> On 1/7/20 10:04 AM, Chris Plummer wrote:
>>> Hi Serguei,
>>>
>>> The reason you don't see a build failure is because the 
>>> implementation of ServiceThread::enqueue_deferred_event() is not in 
>>> a JVMTI file that gets excluded from minimalVM builds, thus it is 
>>> still callable and linkable. If you choose to use NOT_JVMTI_RETURN 
>>> for it in the header file, then you will also need to #ifdef around 
>>> the implementation.
>>>
>>> Chris
>>>
>>> On 1/7/20 9:16 AM, serguei.spitsyn@oracle.com wrote:
>>>> Hi Chris,
>>>>
>>>> The slowdebug minimal build does not fail without NOT_JVMTI_RETURN 
>>>> in the ServiceThread::enqueue_deferred_event().
>>>> I'm curious why and will check if it is really needed.
>>>> If so, will add it as well.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 1/6/20 8:05 PM, serguei.spitsyn@oracle.com wrote:
>>>>> Hi Chris,
>>>>>
>>>>> Good catch.
>>>>> I agree, for consistency the enqueue_event() is better to follow 
>>>>> the JvmtiDeferredEventQueue::enqueue() to avoid slowdebug minimal 
>>>>> build failures.
>>>>>
>>>>> New patch is:
>>>>>
>>>>> diff --git a/src/hotspot/share/prims/jvmtiThreadState.hpp 
>>>>> b/src/hotspot/share/prims/jvmtiThreadState.hpp
>>>>> --- a/src/hotspot/share/prims/jvmtiThreadState.hpp
>>>>> +++ b/src/hotspot/share/prims/jvmtiThreadState.hpp
>>>>> @@ -396,7 +396,7 @@
>>>>>    void set_should_post_on_exceptions(bool val) { 
>>>>> _thread->set_should_post_on_exceptions_flag(val ? JNI_TRUE : 
>>>>> JNI_FALSE); }
>>>>>
>>>>>    // Thread local event queue, which doesn't require taking the 
>>>>> Service_lock.
>>>>> -  void enqueue_event(JvmtiDeferredEvent* event);
>>>>> +  void enqueue_event(JvmtiDeferredEvent* event) NOT_JVMTI_RETURN;
>>>>>    void post_events(JvmtiEnv* env);
>>>>>    void run_nmethod_entry_barriers();
>>>>>  };
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>> On 1/6/20 6:39 PM, Chris Plummer wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> JvmtiDeferredEventQueue::enqueue() is a NOP for minimal builds, 
>>>>>> so it can be called even for minimalVM builds:
>>>>>>
>>>>>>   void enqueue(JvmtiDeferredEvent event) NOT_JVMTI_RETURN;
>>>>>>
>>>>>> The changes for JDK-8212160 seem to have put some wrapper code 
>>>>>> around its use, resulting in 
>>>>>> ServiceThread::enqueue_deferred_event() and 
>>>>>> JvmtiThreadState::enqueue_event() being added. Shouldn't NOP 
>>>>>> implementations also have been done for them?
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 1/6/20 6:35 PM, Chris Plummer wrote:
>>>>>>> Hold your horses. I have questions. Working on them now. Please 
>>>>>>> don't push.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 1/6/20 6:29 PM, coleen.phillimore@oracle.com wrote:
>>>>>>>> This looks trivial.  Thank you for fixing it.
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>> On 1/6/20 9:18 PM, serguei.spitsyn@oracle.com wrote:
>>>>>>>>> Please, review a trivial fix for bug:
>>>>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8236124
>>>>>>>>>
>>>>>>>>> Patch suggested by A. Shipilev:
>>>>>>>>>
>>>>>>>>> diff --git a/src/hotspot/share/code/nmethod.cpp 
>>>>>>>>> b/src/hotspot/share/code/nmethod.cpp
>>>>>>>>> --- a/src/hotspot/share/code/nmethod.cpp
>>>>>>>>> +++ b/src/hotspot/share/code/nmethod.cpp
>>>>>>>>> @@ -1601,7 +1601,7 @@
>>>>>>>>> ServiceThread::enqueue_deferred_event(&event);
>>>>>>>>>      } else {
>>>>>>>>>        // This enters the nmethod barrier outside in the caller.
>>>>>>>>> -      state->enqueue_event(&event);
>>>>>>>>> + JVMTI_ONLY(state->enqueue_event(&event));
>>>>>>>>>      }
>>>>>>>>>    }
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Summary:
>>>>>>>>>   The slowdebug build was broken by the fix of JDK-8212160 
>>>>>>>>> which introduced new function: enqueue_event().
>>>>>>>>>   The fix is to call it only if the JVM TI is enabled.
>>>>>>>>>
>>>>>>>>> Testing:
>>>>>>>>>   Ran slowdebug build locally.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]