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

[Jon Masamitsu]

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.

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
>>
>