RFR(M): 8145221: Use trampolines for i2i and i2c entries in Methods that are stored in CDS archive

[Ioi Lam]

On 3/21/16 11:24 AM, Jiangli Zhou wrote:
>
>> On Mar 21, 2016, at 9:40 AM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto:ioi.lam at oracle.com>> wrote:
>>
>>
>>
>> On 3/18/16 6:43 PM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>>> On Mar 18, 2016, at 4:04 PM, Ioi Lam <ioi.lam at oracle.com 
>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>
>>>>
>>>>
>>>> On 3/18/16 11:52 AM, Jiangli Zhou wrote:
>>>>>
>>>>>>>
>>>>>>> The changes look good overall. Following are my comments and 
>>>>>>> questions.
>>>>>>>
>>>>>>> - src/os_cpu/bsd_x86/vm/thread_bsd_x86.cpp
>>>>>>> - src/os_cpu/linux_aarch64/vm/thread_linux_aarch64.cpp
>>>>>>> - src/os_cpu/linux_sparc/vm/thread_linux_sparc.cpp
>>>>>>> - src/os_cpu/linux_x86/vm/thread_linux_x86.cpp
>>>>>>> - src/os_cpu/solaris_sparc/vm/thread_solaris_sparc.cpp
>>>>>>> - src/os_cpu/solaris_x86/vm/thread_solaris_x86.cpp
>>>>>>> - src/os_cpu/windows_x86/vm/thread_windows_x86.cpp
>>>>>>>   Please place the new CDS-only code under #if INCLUDE_CDS.
>>>>>>>
>>>>>>>  It would be a good idea to assert the address (current pc) is 
>>>>>>> within the generated trampoline entry buffer.
>>>>>> I am not sure what this means. Could you explain?
>>>>>
>>>>>
>>>>> We should assert that the address is in the generated trampoline 
>>>>> code, but not at a random place in the shared space. If the 
>>>>> control dumps to an arbitrary location, it might manifest itself 
>>>>> into a bug that’s hard to debug.
>>>>
>>>> I still don't understand where you want to put the assertion. Could 
>>>> you tell me which file/function you want to add the assert, and 
>>>> what should be put in the assert?
>>>
>>>
>>> For example, assert the pc() is in trampoline code if we get an 
>>> address in the shared space in the following code in 
>>> JavaThread::pd_get_top_frame().
>>>
>>>    69     if (UseSharedSpaces && MetaspaceShared::is_in_shared_space(addr.pc())) {
>>>    70       // In the middle of a trampoline call. Bail out for safety.
>>>    71       // This happens rarely so shouldn't affect profiling.
>>>    72       return false;
>>>    73     }
>>> Something like the following?
>>>
>>> assert(addr.pc() >= MetaspaceShared::cds_i2i_entry_code_buffers() &&
>>>            addr.pc() < MetaspaceShared::cds_i2i_entry_code_buffers() 
>>> + MetaspaceShared::cds_i2i_entry_code_buffer_size(),
>>>            “Unexpected");
>>>
>> The address range you listed is just part of the trampolines (for the 
>> "standard" method entries). More trampolines can be allocated here -- 
>> these are entries that are associated with specific method signatures.
>>
>> 3021 void CDSAdapterHandlerEntry::init() {
>> 3022   assert(DumpSharedSpaces, "used during dump time only");
>> 3023   _c2i_entry_trampoline = 
>> (address)MetaspaceShared::misc_code_space_alloc(SharedRuntime::trampoline_size());
>> 3024   _adapter_trampoline = 
>> (AdapterHandlerEntry**)MetaspaceShared::misc_data_space_alloc(sizeof(AdapterHandlerEntry*));
>> 3025 };
>>
>> In addition, addr.pc() can also be one of the vtable patching methods.
>>
>> So, instead of adding an assert, I think it's better to narrow the 
>> check to something like:
>>
>>      if (UseSharedSpaces && 
>> MetaspaceShared::is_in_shared_misc_code_space(addr.pc())) {
>
> Ok.
>
> BTW, one of my comments in first email suggested to not map the ‘mc’ 
> space RW. We should either allocate the trampoline buffer in the ‘md’ 
> space. Another solution would be to add a new read-write-executable 
> space for the trampoline code. So the above check would check if the 
> 'add.pc()' is in the ‘md’ space or the new space. I’m in favor of 
> adding a new read-write-executable space for the runtime generated code.
>
>
That sounds reasonable. Maybe that can be done in a separate RFE, since 
the changes for trampolines are quite complex already?

Thanks
- Ioi

> Thanks,
> Jiangli
>
>>
>>
>> [...snip...]
>>
>>>>
>>>>>>> - src/share/vm/oops/method.hpp
>>>>>>>   In set_interpreter_entry(), when _i2i_entry and 
>>>>>>> _from_interpreted_entry are not the same as ‘entry’ for shared 
>>>>>>> method?
>>>>>>
>>>>>> The purpose of the check is to avoid writing the same value into 
>>>>>> the shared Method (which would cause the pages to be 
>>>>>> copied-on-write, even if the value does not change).
>>>>>>
>>>>>> 475   void set_interpreter_entry(address entry) {
>>>>>> 476     if (_i2i_entry != entry) {
>>>>>> 477       _i2i_entry = entry;
>>>>>> 478     }
>>>>>> 479     if (_from_interpreted_entry != entry) {
>>>>>> 480       _from_interpreted_entry = entry;
>>>>>> 481     }
>>>>>> 482   }
>>>>>
>>>>> I see set_interpreter_entry() is called from Method initializing 
>>>>> code and link_method(). For a shared method, we should change the 
>>>>> if check to be an assert. Would there be any case where the entry 
>>>>> is not the same as the one in the shared method?
>>>>
>>>> I am actually not sure if for shared methods the i2i_entry and 
>>>> from_interpreter_entry would be set to any other values. E.g., in 
>>>> the future, someone may add a new tracing facility that requires 
>>>> modifying these 2 fields.
>>>>
>>>> I'd rather play safe so we don't overwrite into the shared Method 
>>>> most of the time, but if it does happen it will continue to work.
>>>
>>> This part of the code is more critical. So, we should have a tighter 
>>> control of it. If someone is setting the the entry to a different 
>>> valve than the one in the Method, we need to know what is the new 
>>> entry and understand why. The assertion would force us to exam each 
>>> new case like the one you described in the above. Is that reasonable?
>>>
>> Sounds good. I think we should add the assert so we will catch such 
>> new cases in non-product builds. But for safety, in product builds, 
>> we should set the values as requested. We don't want assumptions made 
>> by this optimization to affect functional correctness.
>>
>> 475   void set_interpreter_entry(address entry) {
>>         if (is_shared()) {
>>           assert(_i2i_entry != entry && _from_interpreted_entry != 
>> entry, "must be");
>>         }
>> 476     if (_i2i_entry != entry) {
>> 477       _i2i_entry = entry;
>> 478     }
>> 479     if (_from_interpreted_entry != entry) {
>> 480       _from_interpreted_entry = entry;
>> 481     }
>> 482   }
>>
>>
>