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

[Igor Veresov]

Looks good!

igor

On 5/12/11 3:55 PM, Tom Rodriguez wrote:
> 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.
>>>>
>>>
>>
>