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

[Ioi Lam]

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