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

[Ioi Lam]

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.

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