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

[Vladimir Kozlov]

I found that we have already such function fat_nop() but I am not sure it is 
correct. I am worried that it is 2 nops in 64-bit VM:

   emit_byte(0x66);
   emit_byte(0x66);
   emit_byte(0x90);
   emit_byte(0x66);
   emit_byte(0x90);

Thanks,
Vladimir

Vladimir Kozlov wrote:
> Christian Thalinger wrote:
>> 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.
> 
> I'm thinking now to add new asm function nop_for_patching() which will 
> generated either addr_nop_5 or stack_bang based on UseAddressNop flag.
> 
> I will add asserts into addr_nop_*().
> 
> Thanks,
> Vladimir
> 
>>
>> -- 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
>>