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

[serguei.spitsyn at oracle.com]

On 10/17/13 11:03 PM, David Holmes wrote:
> 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 ???

Nice catch, David.
I do not see too any of them is really used.
Is it a leftover after the permgen elimination?

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

Right.
The issue is that the oop version is used in the oops_do.
Not sure if we can get rid of oops_do.
It may have an empty body though.


Thanks,
Serguei

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