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

[Jon Masamitsu]

Refactoring turned out well.

Thanks.

Jon

On 5/14/13 2: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
>>>>
>>>
>>
>