RFR(S): 8028107: Kitchensink crashed with EAV

[Vladimir Kozlov]

On 12/4/13 9:53 AM, Christian Thalinger wrote:
> Do I understand the change in compiledIC.cpp right that this makes sure we fall-back to the interpreter when the patching in SharedRuntime::resolve_sub_helper didn’t happen?

Yes, you are correct.

I updated webrev with enum renaming suggested by John. And add early 
bailout (do not patch) if caller nmethod is not 'in_use'.

http://cr.openjdk.java.net/~kvn/8028107/webrev.02/

Thanks,
Vladimir

>
> On Dec 3, 2013, at 6:25 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> http://cr.openjdk.java.net/~kvn/8028107/webrev/
>>
>> https://bugs.openjdk.java.net/browse/JDK-8028107
>>
>> As I explained in previous mail SharedRuntime::resolve_sub_helper() has Lock between saving nmethod pointer in local CompiledICInfo variable and when it is used to create new compiledIC entry.
>> Lock may block for safepoint during which nmethod can change state: it can be deoptimized or unloaded.
>> I looked in other places and did not find anything suspicious. For example, in handle_ic_miss_helper() compiled entry computation and compiledIC setting are done under the same lock.
>>
>> The fix is to check the state of caller and callee nmethods and skip call site patching if any of them is not alive.
>>
>> Note, we have strange/swapped names for functions which check nmethod state:
>>
>>   bool  is_in_use() const { return _state == alive; }
>>   bool  is_alive() const  { return _state == alive || _state == not_entrant; }
>>
>> The dest_entry_point assert in resolve_sub_helper() is mostly useless now but I kept it slightly modified.
>>
>> I was not able to reproduce the problem running kitchensink for 5 days. So I can't verify this fix. As I pointed in the bug report this failure happens about once per year.
>>
>> I also fixed indention in nmethod.cpp but webrev does not handle such changes. Here they are:
>>
>> src/share/vm/code/nmethod.cpp
>> @@ -1660,8 +1660,8 @@
>>            CompiledICHolder* cichk_oop = ic->cached_icholder();
>>            if (cichk_oop->holder_method()->method_holder()->is_loader_alive(is_alive) &&
>>                cichk_oop->holder_klass()->is_loader_alive(is_alive)) {
>> -              continue;
>> -            }
>> +            continue;
>> +          }
>>          } else {
>>            Metadata* ic_oop = ic->cached_metadata();
>>            if (ic_oop != NULL) {
>> @@ -1677,8 +1677,8 @@
>>                ShouldNotReachHere();
>>              }
>>            }
>> -          }
>> -          ic->set_to_clean();
>> +        }
>> +        ic->set_to_clean();
>>        }
>>      }
>>    }
>>
>> Thanks,
>> Vladimir
>