RFR(S): 8028107: Kitchensink crashed with EAV

[Christian Thalinger]

Looks good.

On Dec 4, 2013, at 12:57 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

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