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

[Tom Rodriguez]

So I just discovered something disturbing which is that Thread::oops_do isn't virtual even though it's overridden in JavaThread.  Currently it seems to work ok because they are normally called on JavaThread typed variables but if we ever called oops_do on a JavaThread that was statically typed as Thread then we'd skip the JavaThread::oops_do logic.  It doesn't appear that we ever do this but it was keeping my fix from working as I intended.  I've fixed that and updated my webrev.

tom

On May 12, 2011, at 3:22 PM, Tom Rodriguez wrote:

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