review (S) for 6982370: SIGBUS in jbyte_fill

[Vladimir Kozlov]

Good.

Vladimir

Tom Rodriguez wrote:
> 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.
>