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

[Ioi Lam]

Hi Dean,

Thanks for raising this issue.

My first impression is this probably shouldn't matter (or somehow we can 
get away with this).

Today, the CDS archive already contains executable code in the "misc 
code" section. On Linux/x64, this is typically in the address range 
0x802400000-0x802409000. The code is used to dynamically patch the C++ 
vtables of Metadata types that are stored in the CDS archive. So even 
today, there's a chance that the suspended PC lands in this section.

The code is generated inside MetaspaceShared::generate_vtable_methods 
and looks like this  when you run with -Xshare:on:

(gdb) x/5i 0x802400000
    0x802400000:    mov    $0x0,%eax
    0x802400005:    jmpq   0x8024084d0
    0x80240000a:    mov    $0x1,%eax
    0x80240000f:    jmpq   0x8024084d0
    0x802400014:    mov    $0x2,%eax
    ....
(gdb) x/16i 0x8024084d0
    0x8024084d0:    push   %rsi
    0x8024084d1:    push   %rdi
    0x8024084d2:    mov    %rax,%rdi
    0x8024084d5:    shr    $0x8,%rdi
    0x8024084d9:    shl    $0x3,%rdi
    0x8024084dd:    movabs $0x802000000,%rsi
    0x8024084e7:    add    %rdi,%rsi
    0x8024084ea:    mov    (%rsi),%rsi
    0x8024084ed:    pop    %rdi
    0x8024084ee:    mov    %rsi,(%rdi)
    0x8024084f1:    and    $0xff,%rax
    0x8024084f8:    shl    $0x3,%rax
    0x8024084fc:    add    %rsi,%rax
    0x8024084ff:    pop    %rsi
    0x802408500:    mov    (%rax),%rax
    0x802408503:    jmpq   *%rax

In JDK9, the profiler takes a sample by first calling into 
JavaThread::pd_get_top_frame_for_profiling:

(gdb) where
#0  frame::safe_for_sender (this=0x7ffec63895f0, thread=0x7ffff08ce000)
#1  0x00007ffff69d72a7 in JavaThread::pd_get_top_frame 
(this=0x7ffff08ce000, fr_addr=0x7ffec63897b0, ucontext=0x7ffec6287d00, 
isInJava=true)
#2  0x00007ffff69d70be in JavaThread::pd_get_top_frame_for_profiling 
(this=0x7ffff08ce000, fr_addr=0x7ffec63897b0, ucontext=0x7ffec6287d00,
     isInJava=true)

I tried manually setting frame::_pc to 0x802400000, and it would cause 
frame::safe_for_sender() to return false, and thus would prevent the 
profiler from doing any stack walking.

Also, both the CDS vtable patching code and the i2i trampolines are tail 
calls, so you will never find a PC in them except for the top-most 
frame. This means the check in JavaThread::pd_get_top_frame is enough. 
There's no need to do additional checks after we have started stack walking.

I think the chance of landing in the CDS vtable patching methods, or in 
the trampolines, is pretty small, so that shouldn't bother the sampling 
profiler too much.

Anyway, as part of this bug, we should add a regression test with the 
profiler enabled, and keep calling a method in the CDS archive in a 
tight loop (and manually disable compilation of that method using 
command-line options). The test should never crash, and if possible, it 
should also check that we don't have too many "unknown" samples.

Thanks
- Ioi


On 2/19/16 4:39 PM, Dean Long wrote:
> We have profiling code that will suspend a thread at random points and 
> try to walk the stack.
> I think frame::sender() will get confused if you happen to catch a 
> thread in the trampoline,
> and frame::_pc is in metadata and not the code cache.
>
> dl
>
> On 2/19/2016 2:19 PM, Calvin Cheung wrote:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8145221
>> webrev: http://cr.openjdk.java.net/~ccheung/8145221/webrev.00/
>>
>> This optimization reduces the size of the RW region of the CDS 
>> archive. It also reduces the amount of pages in the RW region that 
>> are actually written into during runtime.
>>
>> The original prototype on the x86_64 platform was done by Ioi 
>> (ioi.lam at oracle.com).
>> I helped porting it to other platforms.
>> Special thanks to Goetz (goetz.lindenmaier at sap.com) who provided the 
>> changes to the ppc platform.
>>
>> Testing:
>>     JPRT with testset hotspot
>>     maunal testing on X86_64, x86_32 and solaris/sparcv9 with 
>> -XX:+PrintAssembly -XX:+PrintInterpreter
>>     built on the Zero platform
>>     tested on the openjdk aarch64 platform
>>     refworkload on various platform (please refer to bug report for 
>> data)
>>
>> thanks,
>> Calvin
>