RFR 8152065: TraceBytecodes breaks the interpreter expression stack
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Mar 18 17:42:05 UTC 2016
Dan, Thank you for the code review and filling out the explanation of
this change.
See below about call_VM_leaf.
On 3/18/16 11:45 AM, Daniel D. Daugherty wrote:
> 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.
Yes.
>
> 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?
I started to change this, thinking the same thing. Most of the calls
to call_VM_leaf don't walk the stack because they don't need to if they
don't safepoint, so they don't set up last_Java_sp. There are a couple
of calls that do set last_Java_sp. On x86, set_last_Java_sp also sets
the fp (frame pointer) which is how the method and bcp are found.
Maybe to be even more correct, these should be passed but I didn't want
to change all the platforms which would require testing platforms that I
don't have.
>
> Thumbs up on the fix. Would be good to nail down some of the
> nagging questions, but not absolutely necessary...
Thank you! I tried to explain this anomaly in the comment in
InterpreterRuntime::trace_bytecode().
Coleen
>
> 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