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

[Vladimir Kozlov]

 > It's a static so it's automatically initialized to 0.

Agree, but explicit initialization to 0 is much better to understand.

nm->compile_kind() is not char so using %c is wrong.

Why report_events(int id, address entry) uses ARRAY_SIZE(_records) as limit?

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.

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