On Mon, 30 Jun 2025 04:42:59 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> The segv/eav happens in the case if JvmtiBreakpoint::_method's class
>> redefined old between getting the Method* from jmethodid in the
>> JvmtiEnv::SetBreakpoint(Method* method, jlocation location) {..} and
>> and actual setting breakpoint in the VM operation VM_ChangeBreakpoints.
>>
>> Here are details:
>> The breakpoint is set in 2 steps.
>> 1) method jvmti_SetBreakpoint(jvmtiEnv* env, jmethodID method, jlocation
>> location) convert jmethodID to Method* and call
>> JvmtiEnv::SetBreakpoint(Method* method, jlocation location)
>> where
>> JvmtiBreakpoint bp(method, location);
>> is created with this Method*
>> Note: it is done while thread is in VM state, so Method can't become is_old
>> while this is done.
>>
>> 2) The VMOp is used to add breakpoint into the list
>> VM_ChangeBreakpoints set_breakpoint(VM_ChangeBreakpoints::SET_BREAKPOINT,
>> &bp);
>> VMThread::execute(&set_breakpoint);
>> to call JvmtiBreakpoints::set_at_safepoint()
>> that can modify JvmtiBreakpoints list and set breakpoint in safepoint
>> without synchronization.
>>
>> So it might be possible that class redefinition VM_RedefineClasses
>> operation that redefine the class with this breakpoint happens between steps
>> 1) and 2)
>> VM_RedefineClasses::redefine_single_class()
>> clear all class-related breakpoints in the JvmtiBreakpoints, however the
>> "problematic" breakpoint is in VMThread queue and thus we are still
>> continue to do this operation.
>> So in the step 2) the the JvmtiBreakpoint with 'is_old' method is added to
>> the JvmtiBreakpoints and breakpoint is set.
>>
>> Then old method mights be purged any time once they are not on the stack
>> and any access to this breakpoint could lead to usage of Metthod* _method
>> pointing to deallocated metaspace.
>>
>> The VM_RedefineClasses clear all breakpoints so it is correct just to don't
>> proceed with current breakpoint also.
>>
>> Looks, like very unlikely but reproducing with stress test after some time.
>> Verified that the crash is not reproduced anymore with corresponding test
>> after the fix.
>>
>> Many thanks to Coleen for detailed explanation of class redefinition.
>
> src/hotspot/share/prims/jvmtiImpl.cpp line 191:
>
>> 189: // the jmethod_id's method was updated if class redefintion happened
>> for this class
>> 190: // after JvmtBreakpoint was created but before JVM_ChangeBreakpoints
>> started
>> 191: // all class breakpoints are cleared during redefinition so don't
>> set/clear this breakpoint
>
> So basically one thread is trying to change this particular BP and it races
> with another thread that performs redefinition. If the redefinition thread
> wins then we are turning this current change into a no-op on the basis that
> the redefinition cleared all BPs anyway so we should not now set this one (if
> that was requested).
>
> It is very unclear to me how the thread that requested the current change
> might respond to that request being ignored.
>
> Please add some punctuation to the comment block as it is very hard to read
> at present. Thanks
Thanks for feedback.
> It is very unclear to me how the thread that requested the current change
> might respond to that request being ignored.
>
The SetBreakpoint returns 'JVMTI_ERROR_NONE' just like method set breakpoint
and the breakpoint was removed later by class redfinition.
BTW The alternative would be to replace _method with new version and set
breakpoint like we first redefine classes and then set breakpoint. The both
approaches looks valid for specification, I think.
> Please add some punctuation to the comment block as it is very hard to read
> at present. Thanks
will do this a little bit later.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174287943
