RFR 8152065: TraceBytecodes breaks the interpreter expression stack

[Coleen Phillimore]

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

> 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
>>>>>>
>>>>
>>