[9] RFR(S): 8169711: CDS does not patch entry trampoline if intrinsic method is disabled

[Tobias Hartmann]

Thanks for the reviews, Vladimir and Ioi!

As Vladimir suggested, I moved the UseFMA check into TemplateInterpreterGenerator::generate_math_entry():
http://cr.openjdk.java.net/~thartmann/8169711/webrev.01/

Best regards,
Tobias

On 17.11.2016 22:31, Ioi Lam wrote:
> Hi Tobias,
> 
> The interpreter changes look OK to me. I'll defer to Vladimir on his opinion on the asserts.
> 
> Thanks
> - Ioi
> 
> On 11/17/16 11:34 AM, Vladimir Kozlov wrote:
>> Hi Tobias,
>>
>> It is a little inconsistent. CRC32 instrinsics check their flag in generate_CRC32* methods.
>> May be we should do the same for FMA instead of assert in generate_math_entry() return NULL if flag is false.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/17/16 4:42 AM, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8169711
>>> http://cr.openjdk.java.net/~thartmann/8169711/webrev.00/
>>>
>>> When dumping metadata with class data sharing (CDS), Method::unlink_method() takes care of removing all entry points of methods that will be shared. The _i2i and _from_interpreted entries are set to the corresponding address in the _cds_entry_table (see AbstractInterpreter::entry_for_cds_method()). This address points to a trampoline in shared space that jumps to the actual (unshared) interpreter method entry at runtime (see AbstractInterpreter::update_cds_entry_table()).
>>>
>>> Intrinsic methods may have a special interpreter entry (for example, 'Interpreter::java_lang_math_fmaF') and if they are shared, their entry points are set to such a trampoline that is patched at runtime to jump to the interpreter stub containing the intrinsic code.
>>>
>>> The problem is that if an intrinsic is enabled during dumping but disabled during re-using the shared archive, the trampoline is not patched and therefore still refers to the old stub address that was only valid during dumping. In debug, we hit the "should be correctly set during dump time" assert in Method::link_method() because the method entries are inconsistent. In product, we crash because we jump to an invalid address through the unpatched trampoline. This problem exists with the FMA, CRC32 and CRC32C intrinsics (see regression test).
>>>
>>> I fixed this by always creating the interpreter method entries for intrinsified methods but replace them with vanilla entries in TemplateInterpreterGenerator::generate_method_entry() if the intrinsic is disabled at runtime. Like this, we patch the trampoline destination address even if the intrinsic is disabled but just execute the Java bytecodes instead of the stub.
>>>
>>> While testing, I noticed that the assert in Method::link_method() is not always triggered (sometimes we just crash). This is because
>>> 1) the "_from_compiled_entry == NULL" check in Method::restore_unshareable_info() is always false and therefore link_method() is not invoked and
>>> 2) in Method::link_method() we only execute the check if the adapter (which is shared) was not yet initialized.
>>>
>>> I filed JDK-8169867 [1] for this because I'm not too familiar with the CDS internals.
>>>
>>> Tested with regression test, JPRT and RBT (running).
>>>
>>> Thanks,
>>> Tobias
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8169867
>>>
>