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

[serguei.spitsyn at oracle.com]

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