RFR(XXS): 8236124: Minimal VM slowdebug build failed after JDK-8212160

[coleen.phillimore at oracle.com]

On 1/7/20 2:25 PM, serguei.spitsyn at 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

>
> 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 at 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 at 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 at oracle.com wrote:
>>>>>>> This looks trivial.  Thank you for fixing it.
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 1/6/20 9:18 PM, serguei.spitsyn at 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>