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

[David Holmes]

On 18/10/2013 3: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?

Even more confusing to me is the fact the neither of these seem to 
actually be used anywhere ???

But if we have the Handle then the oop is redundant AFAICS.

David

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