RFR: 8025834: NPE in Parallel Scavenge with -XX:+CheckUnhandledOops

[serguei.spitsyn at oracle.com]

I forgot to say "thank you" for taking care about this issue!

Thanks,
Serguei

On 10/17/13 10:49 PM, serguei.spitsyn at oracle.com wrote:
> Hi Erik,
>
> The fix looks good in general.
>
> But one thing is confusing in the fix.
> Why do you keep both _class_loader and _class_loader_handle in the 
> JvmtiBreakpoint class?
>
> Also, you need to run the nsk.jdi.testlist and nsk.jdwp.testlist test 
> suites as well.
>
> Thanks,
> Serguei
>
>
> On 10/17/13 2:28 PM, Erik Helin wrote:
>> Hi Mikael and Coleen,
>>
>> thanks for your reviews!
>>
>> On 2013-10-16, Mikael Gerdin wrote:
>>> jvmtiImpl.hpp:
>>> Since clone() uses unhandled oops, and is only supposed to be used
>>> by the VM operation, would it make sense to
>>> assert(SafepointSynchronize::is_at_safepoint())?
>>>
>>> 196   GrowableElement *clone()        {
>>>   197     return new JvmtiBreakpoint(_method, _bci, 
>>> _class_loader_handle);
>> Agree, I've updated the patch. A new webrev is located at:
>> http://cr.openjdk.java.net/~ehelin/8025834/webrev.01/
>>
>> On 2013-10-16, Mikael Gerdin wrote:
>>> jvmtiEnv.cpp:
>>> Have you verified that the generated JVMTI entry point contains a
>>> ResourceMark or is it just not needed?
>>> -  ResourceMark rm;
>>> +  HandleMark hm;
>> The JVMTI entry point does not contain a ResourceMark. However, I have
>> verified that a ResourceMark is not needed for jvmtiEnv::SetBreakpoint
>> nor for jvmtiEnv::ClearBreapoint. The only codes that needs a
>> ResourceMark is JvmtiBreakpoints::prints, but that method already has a
>> ResourceMark.
>>
>> On 2013-10-16, Coleen Phillimore wrote:
>>> Did you run the nsk.jvmti.testlist tests too though?
>> No, I had not run the nsk.jvmti.testlist test, but I have now :)
>>
>> I run both with and without the patch on the latest hsx/hotspot-gc. I
>> also run with and without -XX:+CheckUnhandledOops. The results were all
>> the same: 598 passed an 11 failed (the same tests for all combinations).
>> So, the patch does not introduce any regressions for this test suite.
>>
>> Thanks,
>> Erik
>>
>> On 2013-10-16, Mikael Gerdin wrote:
>>> Erik,
>>>
>>> (it's not necessary to cross-post between hotspot-dev and
>>> hotspot-gc-dev, so I removed hotspot-gc from the CC list)
>>>
>>> On 2013-10-16 18:09, Erik Helin wrote:
>>>> Hi all,
>>>>
>>>> this patch fixes an issue where an oop in JvmtiBreakpoint,
>>>> JvmtiBreakpoint::_class_loader, was found by the unhandled oop 
>>>> detector.
>>>>
>>>> Instead of registering the oop as an unhandled oop, which would have
>>>> worked, I decided to wrap the oop in a handle as long as it is on the
>>>> stack.
>>>>
>>>> A JvmtiBreakpoint is created on the stack by the two methods
>>>> JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreakpoint. This
>>>> JvmtiBreakpoint is only created to carry the Method*, jlocation and 
>>>> oop
>>>> to a VM operation, VM_ChangeBreakpoints. VM_ChangeBreakpoints will, 
>>>> when
>>>> at a safepoint, allocate a new JvmtiBreakpoint on the native heap, 
>>>> copy
>>>> the values from the stack allocated JvmtiBreakpoint and then 
>>>> place/clear the
>>>> newly alloacted JvmtiBreakpoint in
>>>> JvmtiCurrentBreakpoints::_jvmti_breakpoints.
>>>>
>>>> I have updated to the code to check that the public constructor is 
>>>> only
>>>> used to allocate JvmtiBreakpoints on the stack (to warn a future
>>>> programmer if he/she decides to allocate one on the heap). The
>>>> class_loader oop is now wrapped in a Handle for stack allocated
>>>> JvmtiBreakpoints.
>>>>
>>>> Due to the stack allocated JvmtiBreakpoint having the oop in a handle,
>>>> the oops_do method of VM_ChangeBreakpoints can be removed. This also
>>>> makes the oop in the handle safe for use after the VM_ChangeBreakpoint
>>>> operation is finished.
>>>>
>>>> The unhandled oop in the JvmtiBreakpoint allocated on the heap will be
>>>> visited by the GC via jvmtiExport::oops_do ->
>>>> JvmtiCurrentBreakpoints::oops_do -> JvmtiBreakpoints::oops_do ->
>>>> GrowableCache::oops_do -> JvmtiBreakpoint::oops_do, since it is being
>>>> added to.
>>>>
>>>> I've also removed some dead code to simplify the change:
>>>> - GrowableCache::insert
>>>> - JvmtiBreakpoint::copy
>>>> - JvmtiBreakpoint::lessThan
>>>> - GrowableElement::lessThan
>>>>
>>>> Finally, I also formatted JvmtiEnv::ClearBreakpoint and
>>>> Jvmti::SetBreakpoint exactly the same to highlight that they share all
>>>> code except one line. Unfortunately, I was not able to remove this 
>>>> code
>>>> duplication in a good way.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~ehelin/8025834/webrev.00/
>>> jvmtiImpl.hpp:
>>> Since clone() uses unhandled oops, and is only supposed to be used
>>> by the VM operation, would it make sense to
>>> assert(SafepointSynchronize::is_at_safepoint())?
>>>
>>> 196   GrowableElement *clone()        {
>>>   197     return new JvmtiBreakpoint(_method, _bci, 
>>> _class_loader_handle);
>>>
>>> jvmtiImpl.cpp:
>>> No comments.
>>>
>>> jvmtiEnv.cpp:
>>> Have you verified that the generated JVMTI entry point contains a
>>> ResourceMark or is it just not needed?
>>> -  ResourceMark rm;
>>> +  HandleMark hm;
>>>
>>> Otherwise the code change looks good.
>>>
>>>
>>> One thing that you didn't describe here, but which was related to
>>> the bug (which we discussed) was the fact that the old code tried to
>>> "do the right thing" WRT CheckUnhandledOops, but it incorrectly
>>> added a Method*:
>>>
>>> thread->allow_unhandled_oop((oop*)&_method);
>>>
>>> We should take care to find other such places where we try to put a
>>> non-oop in allow_unhandled_oop(), perhaps checking is_oop_or_null in
>>> the unhandled oops code.
>>>
>>> /Mikael
>>>
>>>> Testing:
>>>> - JPRT
>>>> - The four tests that were failing are now passing
>>>>
>>>> Thanks,
>>>> Erik
>>>>
>