RFR 8152065: TraceBytecodes breaks the interpreter expression stack
David Holmes
david.holmes at oracle.com
Fri Mar 18 21:45:41 UTC 2016
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. :(
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