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

[Vladimir Kozlov]

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