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

[Tom Rodriguez]

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.