RFR (S) 8169867 Method::restore_unshareable_info does not invoke Method::link_method

[Ioi Lam]

On 11/28/16 12:18 PM, Vladimir Kozlov wrote:
> Hi Ioi,
>
> Did you have updated webrev?
>
I didn't update the webrev. The only change from the previous webrev is 
the diff below
> And you did not comment on my suggestion:
>
> >> Any suggest for a better name?
> >
> > _adapter_cds_entry ?
>
Thanks for the suggestion.

I think "entry" may be confusing with other use of the word, such as 
_i2i_entry -- in this case this pointer doesn't point to the entry point 
of executable code.

I think I'll just leave the names as is for now, and maybe file an RFE 
to rename it in JDK10.

Thanks
- Ioi

> Thanks,
> Vladimir
>
> On 11/27/16 7:58 PM, Ioi Lam wrote:
>> I found a problem in my previous patch. Here's the fix (on top of he
>> previous patch):
>>
>> diff -r 3404f61c7081 src/share/vm/oops/method.cpp
>> --- a/src/share/vm/oops/method.cpp    Sun Nov 27 19:44:44 2016 -0800
>> +++ b/src/share/vm/oops/method.cpp    Sun Nov 27 19:50:35 2016 -0800
>> @@ -1031,11 +1031,13 @@
>>    // leftover methods that weren't linked.
>>    if (is_shared()) {
>>      address entry = Interpreter::entry_for_cds_method(h_method);
>> -    assert(entry != NULL && entry == _i2i_entry && entry ==
>> _from_interpreted_entry,
>> +    assert(entry != NULL && entry == _i2i_entry,
>>             "should be correctly set during dump time");
>>      if (adapter() != NULL) {
>>        return;
>>      }
>> +    assert(entry == _from_interpreted_entry,
>> +           "should be correctly set during dump time");
>>    } else if (_i2i_entry != NULL) {
>>      return;
>>    }
>>
>> The problem is: if the method has been compiled, then a shared method's
>> _from_interpreted_entry would be different than _i2i_entry (see
>> Method::set_code()).
>>
>> I am not sure if Method::link_method() would ever be called after
>> it's been compiled, but I think it's safer to make the asserts no
>> stronger than before this patch.
>>
>> Thanks
>> - Ioi
>>
>>
>> On 11/20/16 11:53 PM, Tobias Hartmann wrote:
>>> Hi Ioi,
>>>
>>> this looks good to me, the detailed description including the diagram
>>> is very nice and helps to understand the complex implementation!
>>>
>>> For the record: the test mentioned in [1] is part of my fix for
>>> JDK-8169711.
>>>
>>> Best regards,
>>> Tobias
>>>
>>> On 21.11.2016 07:58, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8169867
>>>> http://cr.openjdk.java.net/~iklam/jdk9/8169867_cds_not_calling_link_method.v01/ 
>>>>
>>>>
>>>>
>>>> Thanks to Tobias for finding the bug. I have done the following
>>>>
>>>> + integrated Tobias' suggested fix
>>>> + fixed Method::restore_unshareable_info to call Method::link_method
>>>> + added comments and a diagram to illustrate how the CDS method entry
>>>>    trampolines work.
>>>>
>>>> BTW, I am a little unhappy about the name
>>>> ConstMethod::_adapter_trampoline.
>>>> It's basically an extra level of indirection to get to the adapter.
>>>> However.
>>>> The word "trampoline" usually is used for and extra jump in
>>>> executable code,
>>>> so it may be a little confusing when we use it for a data pointer 
>>>> here.
>>>>
>>>> Any suggest for a better name?
>>>>
>>>>
>>>> Testing:
>>>> [1] I have tested Tobias' TestInterpreterMethodEntries.java class and
>>>>      now it produces the correct assertion. I won't check in this
>>>> test, though,
>>>>      since it won't assert anymore after Tobias fixes 8169711.
>>>>
>>>> # after -XX: or in .hotspotrc: SuppressErrorAt=/method.cpp:1035
>>>> #
>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>> #
>>>> #  Internal Error
>>>> (/home/iklam/jdk/ul/hotspot/src/share/vm/oops/method.cpp:1035),
>>>> pid=16840, tid=16843
>>>> #  assert(entry != __null && entry == _i2i_entry && entry ==
>>>> _from_interpreted_entry) failed:
>>>> #  should be correctly set during dump time
>>>>
>>>> [2] Ran RBT in fastdebug build for
>>>> hotspot/test/:hotspot_all,vm.parallel_class_loading,vm.runtime.testlist 
>>>>
>>>>      All tests passed.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>