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

[Tobias Hartmann]

Hi Ioi,

On 28.11.2016 04:58, 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.

That looks reasonable to me!

Thanks,
Tobias

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