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

[Jiangli Zhou]

Hi Ioi,

> On Nov 21, 2016, at 11:05 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 11/21/16 8:33 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>> 
>> Looks good.
>> 
>> I also have one suggestion. To make it a little more easier to read, could you please fold the 'else if (_i2i_entry != NULL)’ block starting at line 1039 and ‘if (!is_shared())’ block starting at line 1047 into to one block?
>> 1032   if (is_shared()) {
>> 1033     address entry = Interpreter::entry_for_cds_method(h_method);
>> 1034     assert(entry != NULL && entry == _i2i_entry && entry == _from_interpreted_entry,
>> 1035            "should be correctly set during dump time");
>> 1036     if (adapter() != NULL) {
>> 1037       return;
>> 1038     }
>> 1039   } else if (_i2i_entry != NULL) {
>> 1040     return;
>> 1041   }
>> 1042   assert( _code == NULL, "nothing compiled yet" );
>> 1043 
>> 1044   // Setup interpreter entrypoint
>> 1045   assert(this == h_method(), "wrong h_method()" );
>> 1046 
>> 1047   if (!is_shared()) {
>> 1048     assert(adapter() == NULL, "init'd to NULL");
>> 1049     address entry = Interpreter::entry_for_method(h_method);
>> 1050     assert(entry != NULL, "interpreter entry must be non-null");
>> 1051     // Sets both _i2i_entry and _from_interpreted_entry
>> 1052     set_interpreter_entry(entry);
>> 1053   }
> 
> Hi Jiangli,
> 
> The line
> 
>     assert( _code == NULL, "nothing compiled yet" );
> 
> is necessary before we call
> 
>     set_interpreter_entry(entry);
> 
> That's because the _from_interpreted_entry would be different if the method has been compiled.
> 
> So this means I cannot simply move the block starting at #1047 to above #1042.


Ok.

Thanks,
Jiangli

> 
> Thanks
> - Ioi
> 
>> Thanks,
>> Jiangli
>> 
>>> On Nov 20, 2016, at 10:58 PM, Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8169867 <https://bugs.openjdk.java.net/browse/JDK-8169867>
>>> http://cr.openjdk.java.net/~iklam/jdk9/8169867_cds_not_calling_link_method.v01/ <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
>>> 
>> 
>