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

[Vladimir Kozlov]

I updated changes to use fixed fat_nop(). Also changed MachProlog code to use 
masm instructions (have to add subptr_imm32() for that).

http://cr.openjdk.java.net/~kvn/7125136/webrev

Thanks,
Vladimir

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