RFR(S): 8028107: Kitchensink crashed with EAV

[Vladimir Kozlov]

Thank you, Christian

Vladimir

On 12/4/13 5:29 PM, Christian Thalinger wrote:
> 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
>>>
>