Re: [Qemu-devel] [PATCH v6 08/16] target-or32: Add instruction tanslation

[Max Filippov]

On Mon, Jun 25, 2012 at 6:50 AM, Jia Liu <address@hidden> wrote:
> Hi Max,
>
> On Thu, Jun 21, 2012 at 6:24 PM, Max Filippov <address@hidden> wrote:
>> On Thu, Jun 21, 2012 at 6:58 AM, Jia Liu <address@hidden> wrote:
>>> Add OpenRISC instruction tanslation routines.
>>>
>>> Signed-off-by: Jia Liu <address@hidden>
>>
>> [...]
>>
>>> +    case 0x0009:
>>> +        switch (op1) {
>>> +        case 0x03:   /*l.div*/
>>> +            LOG_DIS("l.div r%d, r%d, r%d\n", rd, ra, rb);
>>> +            {
>>> +                int lab0 = gen_new_label();
>>> +                int lab1 = gen_new_label();
>>> +                int lab2 = gen_new_label();
>>> +                TCGv_i32 sr_ove = tcg_temp_local_new_i32();
>>> +                if (rb == 0) {
>>> +                    tcg_gen_ori_tl(cpu_sr, cpu_sr, (SR_OV | SR_CY));
>>> +                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab0);
>>> +                    tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>> +                    gen_exception(dc, EXCP_RANGE);
>>> +                    gen_set_label(lab0);
>>> +                } else {
>>> +                    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_R[rb],
>>> +                                       0x00000000, lab1);
>>> +                    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_R[ra],
>>> +                                       0xffffffff, lab2);
>>> +                    tcg_gen_brcondi_tl(TCG_COND_NE, cpu_R[rb],
>>> +                                       0x80000000, lab2);
>>> +                    gen_set_label(lab1);
>>> +                    tcg_gen_ori_tl(cpu_sr, cpu_sr, (SR_OV | SR_CY));
>>> +                    tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
>>> +                    tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab2);
>>
>> Causes host division by zero/overflow. I'd suggest to brcond to lab3 set 
>> after
>> the final tcg_gen_div.
>
> Causes host division by zero/overflow? Can I handle the host code? I'm
> confused about this.
> May I get more comment about this? Sorry I didn't understand it.

The brcondi in question jumps to the division operation although we
know for sure
that it's going to be a division by zero or an overflow. The host is
not smarter than
we are, it cannot magically divide by zero. So, if the result register value is
undefined in case of division by zero/overflow than you should skip the division
and brcondi to a label lab3, right after it.

>>> +                    gen_exception(dc, EXCP_RANGE);
>>> +                    gen_set_label(lab2);
>>> +                    tcg_gen_div_tl(cpu_R[rd], cpu_R[ra], cpu_R[rb]);

+ gen_set_label(lab3);

>>> +                }
>>> +                tcg_temp_free_i32(sr_ove);
>>> +            }
>>> +            break;

A picture worth a thousand words is the test for this specific case.
I think it can look like this (because SR_OVE is cleared upon reset):

int main(void)
{
    int a, b, c;

    b = 0x80000000;
    c = 0xffffffff;

    __asm
    ("l.div  %0, %1, %2\n"
     : "=r"(a)
     : "r"(b), "r"(c)
    );
    return 0;
}

I believe that this test with the current div implementation would crash qemu.

BTW, shouldn't tests that detect wrong behavior do "return 1" or something
like this to make "make check" fully automatic?

-- 
Thanks.
-- Max