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

[Erik Helin]

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