RFR 8152065: TraceBytecodes breaks the interpreter expression stack
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Mar 18 21:51:06 UTC 2016
On 3/18/16 5:45 PM, David Holmes wrote:
> On 19/03/2016 3:42 AM, Coleen Phillimore wrote:
>>
>> Dan, Thank you for the code review and filling out the explanation of
>> this change.
>
> Yes thanks Dan - I wasn't getting it. So really this just works around
> the real problem - call_VM makes an invalid assumption about what rsp
> points to when storing into _last_Java_fp. :(
It's not invalid. It's deliberately valid all the time except for
trace_bytecode().
Coleen
>
> David
> -----
>
>> 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