RFR (XS): 6843375: Debuggee VM crashes performing mark-sweep-compact

[Stefan Karlsson]

Looks good.

StefanK

On 14 maj 2013, at 20:19, Erik Helin <erik.helin at oracle.com> wrote:

> All,
> 
> Stefan pointed out that after removing the JvmtiBreakpoints::clearall method, I could also remove the JvmtiBreakpoints::clearall_at_safepoint method since it was not used any longer.
> 
> Please see the new webrev at:
> http://cr.openjdk.java.net/~ehelin/6843375/webrev.02/
> 
> Thanks,
> Erik
> 
> On 05/14/2013 11:46 AM, Erik Helin wrote:
>> Jon,
>> 
>> On 05/09/2013 04:49 PM, Jon Masamitsu wrote:
>>> 
>>> On 5/9/13 12:55 AM, Erik Helin wrote:
>>>> Hi Jon,
>>>> 
>>>> thanks for reviewing!
>>>> 
>>>> On 05/08/2013 08:21 PM, Jon Masamitsu wrote:
>>>>> Erik,
>>>>> 
>>>>> Change looks good.
>>>>> 
>>>>> Would you consider changing the name of _breakpoints
>>>>> 
>>>>>> class VM_ChangeBreakpoints : public VM_Operation {
>>>>>> private:
>>>>>>  JvmtiBreakpoints* _breakpoints;
>>>>> 
>>>>> to jvmtiExport_breakpoints?
>>>> 
>>>> Would _jvmti_breakpoints make more sense? The VM_ChangeBreakpoints
>>>> constructor always gets passed
>>>> JvmtiCurrentBreakpoints::_jvmti_breakpoints as argument.
>>> 
>>> I like the second suggestion.
>>>> 
>>>> On 05/08/2013 08:21 PM, Jon Masamitsu wrote:
>>>>> I'm looking for a way to alert
>>>>> the reader of the code that this list of breakpoints is part of
>>>>> (owned if you will) by some other variable so that the reader
>>>>> will not think it is part of the responsibility of VM_ChangeBreakpoints
>>>>> to update it (as it was erroneously done before you fix).
>>>>> Does that make sense?  Is the name change enough?  I was
>>>>> trying to come up with some refactoring of the code to make
>>>>> it more obvious that _breakpoints should not be processes
>>>>> by VM_ChangeBreakpoints but this is the best I could do.
>>>> 
>>>> Another suggestion would be to remove the JvmtiBreakpoints* arguments
>>>> to VM_ChangeBreakpoints and instead directly call
>>>> JvmtiCurrentBreakpoints::get_jvmti_breakpoints to show the reader of
>>>> the code that VM_ChangeBreakpoints always make use of
>>>> JvmtiCurrentBreakpoints::_jvmti_breakpoints.
>>>> 
>>>> That way, one would (hopefully) see the JvmtiExport::oops_do calls
>>>> JvmtiCurrentBreakpoints::oops_do and realize that it is not
>>>> VM_ChangeBreakpoints responsibility to call oops_do.
>>>> 
>>>> What do you think?
>>> 
>>> I like this idea.  I was hoping for something like this but the
>>> idea wouldn't crystallize in my brain.
>> 
>> I've uploaded a new webrev at:
>> http://cr.openjdk.java.net/~ehelin/6843375/webrev.01/
>> 
>> Sorry for the noise in jvmtiImpl.hpp, but I had to move the
>> VM_ChangeBreakpoints class below JvmtiCurrentBreakpoints since
>> VM_ChangeBreakpoints now uses JvmtiCurrentBreakpoints.
>> 
>> I also removed the method JvmtiBreakpoints::clearall since it is not
>> being used anywhere.
>> 
>> How do you think the refactoring turned out?
>> 
>> Thanks,
>> Erik
>> 
>>> Thanks.
>>> 
>>> Jon
>>>> 
>>>> Thanks,
>>>> Erik
>>>> 
>>>>> This is a throw away suggestion if you like.
>>>>> 
>>>>> Jon
>>>>> 
>>>>> 
>>>>> 
>>>>> On 5/8/2013 8:51 AM, Erik Helin wrote:
>>>>>> Hi all,
>>>>>> 
>>>>>> this change fixes a problem when a VM_ChangeBreakpoint operation is
>>>>>> enqueued on the VMOperationQueue while at the same time as a GC is
>>>>>> running. The GC will process the strong roots and will call both
>>>>>> VMOperationQueue::oops_do (via Threads::oops_do) and
>>>>>> JvmtiExport::oops_do.
>>>>>> 
>>>>>> VMOperationQueue::oops_do will call VM_ChangeBreakpoint::oops_do which
>>>>>> calls JvmtiBreakpoints::oops_do. The problem is that
>>>>>> JvmtiExport::oops_do calls JvmtiCurrentBreakpoints::oops_do which
>>>>>> points to the same JvmtiBreakpoints as the one VM_ChangeBreakpoint
>>>>>> just called oops_do on.
>>>>>> 
>>>>>> This change remove the code VM_ChangeBreakpoint that calls
>>>>>> JvmtiBreakpoints::oops_do. All the GCs will visit the breakpoints via
>>>>>> JvmtiExport.
>>>>>> 
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~ehelin/6843375/webrev/
>>>>>> 
>>>>>> Testing:
>>>>>> JPRT
>>>>>> 
>>>>>> Bug:
>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6843375
>>>>>> 
>>>>>> Thanks,
>>>>>> Erik
>