RFR 8152065: TraceBytecodes breaks the interpreter expression stack

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Mar 18 15:45:34 UTC 2016 

On 3/18/16 8:02 AM, Coleen Phillimore wrote:
> On 3/18/16 8:41 AM, David Holmes wrote:
>> On 18/03/2016 9:43 PM, Coleen Phillimore wrote:
>>>
>>>
>>> On 3/17/16 10:09 PM, David Holmes wrote:
>>>>
>>>> On 18/03/2016 11:34 AM, Coleen Phillimore wrote:
>>>>>
>>>>> Hi,  Thank you for looking at this.
>>>>>
>>>>> IRT_LEAF and JRT_LEAF are almost identical, just like JRT_ENTRY and
>>>>> IRT_ENTRY.  It's just the convention in interpreterRuntime.cpp to use
>>>>> the IRT version.
>>>>
>>>> Sure - but then why the move from SharedRuntime to IntepreterRuntime ?
>>>
>>> Because it's used by the interpreter only.  That's the first place to
>>> look for it so it's where it belongs.
>>
>> I'll take your word for it - I have no idea what belongs in 
>> sharedRuntime. :)
>>
>> But I'm still confused about the fix. How does the change to being a 
>> leaf method fix the rsp problem? And what is the connection with 
>> safepoints?
> So the IRT/JRT_ENTRY macro has a ThreadInVMFromJava scoped object, 
> where the destructor does SafepointSynchronize::block.  So there's a 
> safepoint check at the IRT/JRT_END of these functions (JRT_BLOCK_END 
> also).
>
> On x86, rsp - the stack pointer- is also the top of the expression 
> stack.   When we do a call_VM, we save rsp to last_Java_sp in the 
> frame.  The C++ frame.cpp code uses last_Java_sp to find oops on the 
> expression stack.
>
> I verified that all the other calls into the call_VM() leave the 
> correct state on the stack, but trace_bytecode pushes registers to 
> save before calling call_VM.
>
> Look for trace_bytecode in 
> src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp.  It pushes the 4 
> argument registers to the stack.  The failure I was debugging showed 
> that these were causing the wrong oops to be found on the expression 
> stack with Tom Benson's debugging code.
>
> The alternatives were to change all the cpu dependent code to not pass 
> any arguments to trace_bytecode, since tos can be retrieved with 
> frame::interpreter_frame_expression_stack_at(0), but I didn't really 
> want to do that.  I can't test a lot of platforms!  Another choice was 
> to copy the guts of call_VM into the trace_bytecode generator so that 
> last_Java_sp is saved before the call to vm.  I briefly looked at 
> this, which is why I noticed that there were incorrect comments in the 
> code.  There are also way too many call_VM variants on x86 at least.  
> All seem needed.
>
> So making trace_bytecode IRT_LEAF is the simplest fix so that we could 
> get to the bottom of these other reported bugs.
>
> thanks,
> Coleen

 > open webrev at http://cr.openjdk.java.net/~coleenp/8152065.01/webrev

src/cpu/aarch64/vm/templateInterpreterGenerator_aarch64.cpp
src/cpu/ppc/vm/templateInterpreterGenerator_ppc.cpp
src/cpu/sparc/vm/templateInterpreterGenerator_sparc.cpp
src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp
src/share/vm/interpreter/bytecodeInterpreter.cpp
     No comments on these renames.

src/cpu/x86/vm/frame_x86.hpp
     No comments (comment removal).

src/cpu/x86/vm/interp_masm_x86.cpp
     No comments (name typo in mesg).

src/share/vm/interpreter/interpreterRuntime.cpp
     No comments (comment removal).

     trace_bytecode() function moves to here and changes from
     JRT_ENTRY -> IRT_LEAF.

src/share/vm/interpreter/interpreterRuntime.hpp
     No comments.

src/share/vm/runtime/sharedRuntime.cpp
     trace_bytecode() function moves from here and changes from
     JRT_ENTRY -> IRT_LEAF.

src/share/vm/runtime/sharedRuntime.hpp
     No comments.


So the old version of trace_bytecode() was marked JRT_ENTRY which
meant that the built-in "ThreadInVMfromJava __tiv(thread)" caused
the caller of trace_bytcode() to potentially safepoint at the end
of the call to trace_bytecode() when ThreadInVMfromJava is destroyed.

src/cpu/x86/vm/templateInterpreterGenerator_x86.cpp:

#ifndef _LP64
<snip>
#else
   __ push(state);
   __ push(c_rarg0);
   __ push(c_rarg1);
   __ push(c_rarg2);
   __ push(c_rarg3);
   __ mov(c_rarg2, rax);  // Pass itos
#ifdef _WIN64
   __ movflt(xmm3, xmm0); // Pass ftos
#endif
   __ call_VM(noreg,
              CAST_FROM_FN_PTR(address, SharedRuntime::trace_bytecode),
              c_rarg1, c_rarg2, c_rarg3);

// So the JRT_ENTRY marking of SharedRuntime::trace_bytecode() means
// that we can go to a safepoint just before call_VM() returns and
// that safepoint might be confused by the regs pushed above.

   __ pop(c_rarg3);
   __ pop(c_rarg2);
   __ pop(c_rarg1);
   __ pop(c_rarg0);
   __ pop(state);
   __ ret(0);                                   // return from result 
handler
#endif // _LP64


So if I understand what you said above, the safepoint code is
expecting last_Java_sp to refer to the interpreter expression
stack and instead it finds four registers on the top of the
stack.

Of course, now that trace_bytecode() is marked as an IRT_LEAF,
I start wondering if we should be using one of the "leaf" variants
of call_VM(), but I see that call_VM_leaf() isn't used anywhere
in templateInterpreterGenerator_x86.cpp. Sigh... Do you happen
to know why we shouldn't use a leaf variant?

Thumbs up on the fix. Would be good to nail down some of the
nagging questions, but not absolutely necessary...

Dan



>
>> Thanks,
>> David
>>
>>> Coleen
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> I guess someday we should get rid of the IRT versions.  I'm not sure
>>>>> what the history was behind them.
>>>>>
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 3/17/16 8:47 PM, David Holmes wrote:
>>>>>> On 18/03/2016 10:08 AM, Jiangli Zhou wrote:
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> This looks like a safe fix, but I probably can’t serve as a very 
>>>>>>> good
>>>>>>> reviewer for this area. I have a question. Would ‘JRT_LEAF’ work 
>>>>>>> also
>>>>>>> in this case?
>>>>>>
>>>>>> That was my question too!
>>>>>>
>>>>>> David
>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>
>>>>>>>> On Mar 17, 2016, at 2:39 PM, Coleen Phillimore
>>>>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Summary: Move trace_bytecode to InterpreterRuntime and make
>>>>>>>> trace_bytecode an IRT_LEAF so that safepoints are not allowed.
>>>>>>>>
>>>>>>>> open webrev at 
>>>>>>>> http://cr.openjdk.java.net/~coleenp/8152065.01/webrev
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8152065
>>>>>>>>
>>>>>>>> Tested with test program for bug
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8151256 and runThese with
>>>>>>>> -XX:+TraceBytecodes for a few minutes (output file got too big).
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>
>>>>>
>>>
>

More information about the hotspot-runtime-dev mailing list