review for 6996747: SIGSEGV in nmethod::cleanup_inline_caches / CompiledIC::verify

[Tom Rodriguez]

Thanks Vladimir and Igor.

tom

On May 12, 2011, at 4:48 PM, Vladimir Kozlov wrote:

> Looks good.
> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> On May 12, 2011, at 4:11 PM, Vladimir Kozlov wrote:
>>>> It's a static so it's automatically initialized to 0.
>>> Agree, but explicit initialization to 0 is much better to understand.
>> Ok.
>>> nm->compile_kind() is not char so using %c is wrong.
>> Oops.  My brain saw char and ran with it.  I switch to %s and added check for NULL.
>>> Why report_events(int id, address entry) uses ARRAY_SIZE(_records) as limit?
>> Leftovers again.
>>> report_events() misses the check for empty records. May be do memset to 0 after creating the buffer and then check for 0 in report_events.
>> Good idea.
>> tom
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> On May 12, 2011, at 3:41 PM, Vladimir Kozlov wrote:
>>>>> Memory leak since possibly_sweep() could be called several time. You need to free or reset previous ring buffer.
>>>> I converted from a static buffer to a malloc'ed one and forgot to add the guard so it's only init'ed once.
>>>>> Missing _sweep_index initialization.
>>>> It's a static so it's automatically initialized to 0.
>>>>> SweeperRecord::kind is not printed.
>>>> I added that.
>>>>> Should it be assert/guarantee? :
>>>>> 
>>>>> +    if (m != nm->method()) {
>>>>> +      tty->print_cr("method oop changed during lock acquire: " INTPTR_FORMAT " != " INTPTR_FORMAT, m, nm->method());
>>>>> +    }
>>>> This should have been deleted.  It was debug code I used to confirm the bug.
>>>> tom
>>>>> Vlaidmir
>>>>> 
>>>>> Tom Rodriguez wrote:
>>>>>> http://cr.openjdk.java.net/~never/6996747
>>>>>> 149 lines changed: 149 ins; 0 del; 0 mod; 10537 unchg
>>>>>> 6996747: SIGSEGV in nmethod::cleanup_inline_caches / CompiledIC::verify
>>>>>> Reviewed-by:
>>>>>> When the sweeper is processing an nmethod it's possible for a
>>>>>> safepoint to occur while acquiring locks to clean the inline caches.
>>>>>> This can allow the nmethod to be unloaded in the middle of processing
>>>>>> it which can result in assertion failures or crashes.  I considered
>>>>>> modifying the locks to skip the safepoint check but it would require
>>>>>> changing CompiledIC_lock, InlineCacheBuffer_lock and VtableStubs_lock
>>>>>> which seems risky.  Instead I keep track of the currently nmethod in
>>>>>> the CompiledThread and scan it when a GC occurs.  I also included some
>>>>>> sweeper logging code that I wrote while debugging this.  Tested with
>>>>>> failing test from report though we'll need big apps runs to confirm
>>>>>> that there aren't other issues.