x86: BT/BTS/BTR/BTC: ZF flag is unaffected

[Daniil Troshkov]

Hello!

Bug was found in qemu.git.
See target-i386/translate.c:

    case 0x1ba: /* bt/bts/btr/btc Gv, im */
        ot = dflag + OT_WORD;
        modrm = ldub_code(s->pc++);
        op = (modrm >> 3) & 7;
        mod = (modrm >> 6) & 3;
        rm = (modrm & 7) | REX_B(s);
        if (mod != 3) {
            s->rip_offset = 1;
            gen_lea_modrm(s, modrm, &reg_addr, &offset_addr);
            gen_op_ld_T0_A0(ot + s->mem_index);
        } else {
            gen_op_mov_TN_reg(ot, 0, rm);
        }
        /* load shift */
        val = ldub_code(s->pc++);
        gen_op_movl_T1_im(val);
        if (op < 4)
            goto illegal_op;
        op -= 4;
        goto bt_op;
    case 0x1a3: /* bt Gv, Ev */
        op = 0;
        goto do_btx;
    case 0x1ab: /* bts */
        op = 1;
        goto do_btx;
    case 0x1b3: /* btr */
        op = 2;
        goto do_btx;
    case 0x1bb: /* btc */
        op = 3;
    do_btx:
        ot = dflag + OT_WORD;
        modrm = ldub_code(s->pc++);
        reg = ((modrm >> 3) & 7) | rex_r;
        mod = (modrm >> 6) & 3;
        rm = (modrm & 7) | REX_B(s);
        gen_op_mov_TN_reg(OT_LONG, 1, reg);
        if (mod != 3) {
            gen_lea_modrm(s, modrm, &reg_addr, &offset_addr);
            /* specific case: we need to add a displacement */
            gen_exts(ot, cpu_T[1]);
            tcg_gen_sari_tl(cpu_tmp0, cpu_T[1], 3 + ot);
            tcg_gen_shli_tl(cpu_tmp0, cpu_tmp0, ot);
            tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
            gen_op_ld_T0_A0(ot + s->mem_index);
        } else {
            gen_op_mov_TN_reg(ot, 0, rm);
        }
    bt_op:
        tcg_gen_andi_tl(cpu_T[1], cpu_T[1], (1 << (3 + ot)) - 1);
        switch(op) {
        case 0:
            tcg_gen_shr_tl(cpu_cc_src, cpu_T[0], cpu_T[1]);
            tcg_gen_movi_tl(cpu_cc_dst, 0); <<<<<<<<<<<<<<<<<<<<<< always set zf
            break;
        case 1:
            tcg_gen_shr_tl(cpu_tmp4, cpu_T[0], cpu_T[1]);
            tcg_gen_movi_tl(cpu_tmp0, 1);
            tcg_gen_shl_tl(cpu_tmp0, cpu_tmp0, cpu_T[1]);
            tcg_gen_or_tl(cpu_T[0], cpu_T[0], cpu_tmp0);
            break;
        case 2:
            tcg_gen_shr_tl(cpu_tmp4, cpu_T[0], cpu_T[1]);
            tcg_gen_movi_tl(cpu_tmp0, 1);
            tcg_gen_shl_tl(cpu_tmp0, cpu_tmp0, cpu_T[1]);
            tcg_gen_not_tl(cpu_tmp0, cpu_tmp0);
            tcg_gen_and_tl(cpu_T[0], cpu_T[0], cpu_tmp0);
            break;
        default:
        case 3:
            tcg_gen_shr_tl(cpu_tmp4, cpu_T[0], cpu_T[1]);
            tcg_gen_movi_tl(cpu_tmp0, 1);
            tcg_gen_shl_tl(cpu_tmp0, cpu_tmp0, cpu_T[1]);
            tcg_gen_xor_tl(cpu_T[0], cpu_T[0], cpu_tmp0);
            break;
        }
        s->cc_op = CC_OP_SARB + ot;
        if (op != 0) {
            if (mod != 3)
                gen_op_st_T0_A0(ot + s->mem_index);
            else
                gen_op_mov_reg_T0(ot, rm);
            tcg_gen_mov_tl(cpu_cc_src, cpu_tmp4);
            tcg_gen_movi_tl(cpu_cc_dst, 0); <<<<<<<<<<<<<<<<<<<<<< always set zf
        }
        break;

always set zf...

There is fixed patch.

[Peter Maydell (pmaydell)]

It would be helpful if you could submit patches in line with the guidance documented on the wiki:
http://wiki.qemu.org/Contribute/SubmitAPatch

In particular, patches should be sent to the mailing list in the right format, and we cannot apply any patch without a signed-off-by line.

Thanks.

[Avi Kivity (avi-redhat)]

On 12/14/2011 06:08 PM, malc wrote:
> On Wed, 14 Dec 2011, Daniil Troshkov wrote:
>
> > Public bug reported:
> >
> > Hello!
> >
> > Bug was found in qemu.git.
> > See target-i386/translate.c:
> >
>
> [..snip..]
>
> Intel's documentation doesn't cover this, AMD's says that ZF is undefined, so,
> question is: why do you think QEMU is wrong here?

The Intel documentation states that ZF is unaffected.

--
error compiling committee.c: too many arguments to function

[Avi Kivity (avi-redhat)]

On 12/14/2011 06:22 PM, malc wrote:
> On Wed, 14 Dec 2011, Avi Kivity wrote:
>
> > On 12/14/2011 06:08 PM, malc wrote:
> > > On Wed, 14 Dec 2011, Daniil Troshkov wrote:
> > >
> > > > Public bug reported:
> > > >
> > > > Hello!
> > > >
> > > > Bug was found in qemu.git.
> > > > See target-i386/translate.c:
> > > >
> > >
> > > [..snip..]
> > >
> > > Intel's documentation doesn't cover this, AMD's says that ZF is undefined, so,
> > > question is: why do you think QEMU is wrong here?
> >
> > The Intel documentation states that ZF is unaffected.
> >
>
> Right, i was blind, anyways, AMD disagrees.
>

Best to be conservative here.

--
error compiling committee.c: too many arguments to function

[Daniil Troshkov (troshkovdanil)]

>Best to be conservative here.
What is it means?

[Avi Kivity (avi-redhat)]

On 12/14/2011 06:33 PM, malc wrote:
> >
> > Best to be conservative here.
> >
>
> Point being, any code that relies on it being in any particular state is
> broken (potentially, on AMD chips)

Yes of course, but not all software is written to be portable. Probably
the only thing that will break here is a vendor test suite, but even so,
if we can comply to the spec, we should.

--
error compiling committee.c: too many arguments to function

[Thomas Huth (th-huth)]

Looking at the previous comments ... is there anything left to do here? Or can we close this bug nowadays?

[Launchpad Janitor (janitor)]

[Expired for QEMU because there has been no activity for 60 days.]