Request for reviews (S): 7125136: SIGILL on linux amd64 in gc/ArrayJuggle/Juggle29

[Christian Thalinger]

On Feb 14, 2012, at 4:56 PM, Vladimir Kozlov wrote:

> Thank you, Christian
> 
> We can't use address nop on old cpus (amd before ss2 and intel before p6, see vm_version_x86.cpp).

Right, I missed that.  It would be a good idea to have an assert or guarantee checking UseAddressNop in these methods.

-- Chris

> I will look on masm definition in MachProlog.
> 
> Thanks,
> Vladimir
> 
> On 2/14/12 1:08 AM, Christian Thalinger wrote:
>> 
>> On Feb 14, 2012, at 5:02 AM, Vladimir Kozlov wrote:
>> 
>>> http://cr.openjdk.java.net/~kvn/7125136/webrev
>>> 
>>> 7125136: SIGILL on linux amd64 in gc/ArrayJuggle/Juggle29
>>> 
>>> Saving RBP register on nmethod's entry broke nmethod's verified entry patching when it become non-entrant. There is big comment in MachPrologNode::emit() about first instruction which should be at lest 5 bytes long. And push(rbp) is one byte instruction. VerifyFPU code also broken (first instruction is pushf). The same with C1 generated code with VerifyFPU and C1Breakpoint.
>>> 
>>> The only reason we did not noticed this until now is stack bang instruction is usually generated first and it is big (store to stack with big offset). But C2 does not generated it if compiled stack frame is small and no calls in compiled method.
>>> 
>>> For C2 moved saving EBP after ESP adjustment. And other cleanup in prolog code.
>>> For C1 generated stack bang with small offset (-256) first if needed.
>> 
>> Why are we using bang_stack_with_offset in this case?  How about addr_nop_5?
>> 
>> It would be better if you would define masm at the beginning of MachPrologNode::emit instead of doing it multiple times in if clauses.
>> 
>> Otherwise this looks good.
>> 
>> -- Chris
>> 
>>> 
>>> Verified by examining generated code.
>>> 
>>> SPARC code is safe since we generate 1 instruction (load from 0) for patching and use signal handler.
>>> 
>>> Thanks,
>>> Vladimir
>>