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

[David Holmes]

On 18/10/2013 5:58 PM, serguei.spitsyn at oracle.com wrote:
> On 10/18/13 12:24 AM, serguei.spitsyn at oracle.com wrote:
>> 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?
>
> Maybe this is a rush judging.
> It depends on the closure->do_oop() that is used for traversing
> I thought that the KeepAliveClosure is used below (basing on the comment).
>
> class JvmtiBreakpoint : public GrowableElement {
>    . . .
>    void oops_do(OopClosure* f)     {
>      // Mark the method loader as live
>      f->do_oop(&_class_loader);
>    }
>
> This oops_do() is not needed if we have handle instead of oop.

Ah! Maybe the only purpose of keeping the class_loader (whether oop or 
Handle) is that it is kept alive outside of its normal lifecycle.

But still we should only need the Handle or the Oop, not both. And if 
there is no oop we should not need an oops_do.

David

>
>>
>>>
>>> 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.
>
> Getting rid of the oops_do will require more cleanup that needs to be
> accurate.
>
>
> Thanks,
> Serguei
>
>>
>>
>> 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
>>>>>>>
>>>>
>>
>