review (S) for 6982370: SIGBUS in jbyte_fill

[Tom Rodriguez]

Thanks.  I've updated the webrev and rerun the test.

tom

On Sep 3, 2010, at 1:25 PM, Vladimir Kozlov wrote:

> Tom Rodriguez wrote:
>>> Could you use shifts?:
>>> 
>>>     __ sllx(O3, 60, O3);
>>>     __ srlx(O3, 60, O3);
>> Yep.  You mean 48 don't you?
> 
> Yes, sorry mmy mistake.
> 
>>> instead of:
>>> 
>>> 1641       __ set(0xffff, O3); << 2 instructions
>>> 1642       __ and3(value, O3, value);
>>> 
>>> Tom, sorry I don't like your changes which increase the code.
>> The normal path doesn't have to split the loads since it knows they are aligned so it seems like a waste to emit extra stores there just so it can be used in the case where it's very small.  I could consolidate the slow path more but I don't want to make the main path slower.
> 
> OK, I got it. Now I agree with your code. It is good to go.
> 
> Thanks,
> Vladimir
> 
>> tom
>>> How about this:
>>> 
>>>   __ BIND(L_fill_4_bytes);
>>>   __ brx(Assembler::zero, false, Assembler::pt, L_fill_2_bytes);
>>>   if (t == T_BYTE || t == T_SHORT) {
>>>     __ delayed()->andcc(count, 1<<(shift-1), G0);
>>> +     if (t == T_SHORT) {
>>> +       __ sth(value, to, 0);
>>> +       __ sth(value, to, 2);
>>> +     } else {
>>> +       __ stb(value, to, 0);
>>> +       __ stb(value, to, 1);
>>> +       __ stb(value, to, 2);
>>> +       __ stb(value, to, 3);
>>> +     }
>>>   } else {
>>>     __ delayed()->nop();
>>> +     __ stw(value, to, 0);
>>>   }
>>> -   __ stw(value, to, 0);
>>>   if (t == T_BYTE || t == T_SHORT) {
>>>     __ inc(to, 4);
>>>     // fill trailing 2 bytes
>>>     __ andcc(count, 1<<(shift-1), G0); // in delay slot of branches
>>>     __ BIND(L_fill_2_bytes);
>>>     __ brx(Assembler::zero, false, Assembler::pt, L_fill_byte);
>>>     __ delayed()->andcc(count, 1, count);
>>> -     __ sth(value, to, 0);
>>>     if (t == T_BYTE) {
>>> +       __ stb(value, to, 0);
>>> +       __ stb(value, to, 1);
>>>       __ inc(to, 2);
>>>       // fill trailing byte
>>>       __ andcc(count, 1, count);  // in delay slot of branches
>>>       __ BIND(L_fill_byte);
>>>       __ brx(Assembler::zero, false, Assembler::pt, L_exit);
>>>       __ delayed()->nop();
>>>       __ stb(value, to, 0);
>>>     } else {
>>> +       __ sth(value, to, 0);
>>>       __ BIND(L_fill_byte);
>>>     }
>>>   } else {
>>>     __ BIND(L_fill_2_bytes);
>>>   }
>>>   __ BIND(L_exit);
>>> 
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/6982370
>>>> 6982370: SIGBUS in jbyte_fill
>>>> Summary:
>>>> Reviewed-by:
>>>> In the new fill routines copy less than 8 bytes long skip over the
>>>> main code and go the final cleanup stage.  On sparc this isn't right
>>>> since it still hasn't accounted for alignment.  The fix is to have a
>>>> special path for handling that.  The code doesn't crash on 32 bit
>>>> sparc because handling of misaligned stores seems to be enabled.
>>>> Additionally I noticed a problem with the logic for clearing the high
>>>> bits of a short value.  The logic I wrote works correctly for clean
>>>> ints but doesn't work for 64 bit signed extended ints resulting in
>>>> invalid fill values in some cases.  Tested with failing nightly and
>>>> new test case to exercise the various alignments.