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

[Tom Rodriguez]

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.