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

[Tom Rodriguez]

Indeed I did.  I was going to put asserts into set_scanned_nmethod to make sure it only toggled between NULL and non-null but it didn't seem necessary.  Actually I just discovered a problem with this change.  Hold off on reviews for a bit.

tom

On May 12, 2011, at 3:17 PM, Igor Veresov wrote:

> 307   NMethodMarker() {
> 308     _thread->set_scanned_nmethod(NULL);
> 309   }
> 
> ^^ did you mean this to be a destructor?
> 
> Otherwise looks good.
> 
> igor
> 
> On 5/12/11 2:31 PM, 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.
>> 
>