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

[Vladimir Kozlov]

Yes, the fat_nop() sequence is wrong. It was from old Guide, in new version of 
the Guide address nops are recommended. So I will change it and use in my code.

Vladimir

Vladimir Kozlov wrote:
> 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
>>>