[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (Jan Kiszka). |
Date: |
Mon, 18 Aug 2008 09:13:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
andrzej zaborowski wrote:
> 2008/7/16 Jan Kiszka <address@hidden>:
>> Laurent Desnogues wrote:
>>> On Wed, Jul 16, 2008 at 2:04 PM, Andreas Schwab <address@hidden> wrote:
>>>> Andrzej Zaborowski <address@hidden> writes:
>>>>
>>>>> void OPPROTO op_tasb_rN(void)
>>>>> {
>>>>> - cond_t(*(int8_t *) env->gregs[PARAM1] == 0);
>>>>> - *(int8_t *) env->gregs[PARAM1] |= 0x80;
>>>>> + cond_t((env->gregs[PARAM1] && 0xff) == 0);
>>>>> + *(int8_t *) &env->gregs[PARAM1] |= 0x80;
>>>> That does not make any sense at all. The TAS insn operates on memory,
>>>> not on a register (atomic operations only make sense on memory anyway).
>>> SH4 documentation says this:
>>>
>>> TAS.B @Rn
>>> If (Rn) = 0, 1 → T, else 0 → T
>>> 1 → MSB of (Rn)
>>>
>>> So indeed it looks like Jan and Andrzej patch is wrong.
>> At least the audience is finally listening. ;)
>>
>> Is this one better?
>
> I suspect one of these may be more correct, but I haven't seen the
> docs. The below, like the original version, assumes that if the store
> generates some kind of trap, the flag is still affected. Otherwise
> cond_t needs to be the last.
>
> diff --git a/target-sh4/op.c b/target-sh4/op.c
> --- a/target-sh4/op.c
> +++ b/target-sh4/op.c
> @@ -592,13 +592,6 @@ void OPPROTO op_shlr16_Rn(void)
> RETURN();
> }
>
> -void OPPROTO op_tasb_rN(void)
> -{
> - cond_t((env->gregs[PARAM1] & 0xff) == 0);
> - *(int8_t *) &env->gregs[PARAM1] |= 0x80;
> - RETURN();
> -}
> -
> void OPPROTO op_movl_T0_rN(void)
> {
> env->gregs[PARAM1] = T0;
> diff --git a/target-sh4/translate.c b/target-sh4/translate.c
> --- a/target-sh4/translate.c
> +++ b/target-sh4/translate.c
> @@ -1077,7 +1077,12 @@ void _decode_opc(DisasContext * ctx)
> gen_op_shlr16_Rn(REG(B11_8));
> return;
> case 0x401b: /* tas.b @Rn */
> - gen_op_tasb_rN(REG(B11_8));
> + gen_op_movl_rN_T0(REG(B11_8));
> + gen_op_movl_T0_T1();
> + gen_op_ldub_T0_T0(ctx);
> + gen_op_cmp_eq_imm_T0(0);
> + gen_op_or_imm_T0(0x80);
> + gen_op_stb_T0_T1(ctx);
> return;
> case 0xf00d: /* fsts FPUL,FRn - FPSCR: Nothing */
> gen_op_movl_fpul_FT0();
>
> or
>
> diff --git a/target-sh4/op.c b/target-sh4/op.c
> --- a/target-sh4/op.c
> +++ b/target-sh4/op.c
> @@ -592,13 +592,6 @@ void OPPROTO op_shlr16_Rn(void)
> RETURN();
> }
>
> -void OPPROTO op_tasb_rN(void)
> -{
> - cond_t((env->gregs[PARAM1] & 0xff) == 0);
> - *(int8_t *) &env->gregs[PARAM1] |= 0x80;
> - RETURN();
> -}
> -
> void OPPROTO op_movl_T0_rN(void)
> {
> env->gregs[PARAM1] = T0;
> diff --git a/target-sh4/op_mem.c b/target-sh4/op_mem.c
> --- a/target-sh4/op_mem.c
> +++ b/target-sh4/op_mem.c
> @@ -76,3 +76,10 @@ void glue(op_stfq_DT0_T1, MEMSUFFIX) (void) {
> glue(stfq, MEMSUFFIX) (T1, DT0);
> RETURN();
> }
> +
> +void glue(op_tasb_Rn, MEMSUFFIX) (void) {
> + uint8_t val = glue(ldub, MEMSUFFIX) (env->gregs[PARAM1]);
> + cond_t(val == 0);
> + glue(stb, MEMSUFFIX) (env->gregs[PARAM1], val | 0x80);
> + RETURN();
> +}
> diff --git a/target-sh4/translate.c b/target-sh4/translate.c
> --- a/target-sh4/translate.c
> +++ b/target-sh4/translate.c
> @@ -80,6 +80,10 @@ static void sh4_translate_init()
> gen_op_st##width##_##reg##_T1_raw(); \
> }
>
> +void gen_op_tasb_Rn(DisasContext *ctx, int reg) {
> + gen_op_tasb_Rn_raw(reg);
> +}
> +
> #else
>
> #define GEN_OP_LD(width, reg) \
> @@ -93,6 +97,13 @@ static void sh4_translate_init()
> else gen_op_st##width##_##reg##_T1_user();\
> }
>
> +void gen_op_tasb_Rn(DisasContext *ctx, int reg) {
> + if (ctx->memidx)
> + gen_op_tasb_Rn_kernel(reg);
> + else
> + gen_op_tasb_Rn_user(reg);
> +}
> +
> #endif
>
> GEN_OP_LD(ub, T0)
> @@ -1077,7 +1088,7 @@ void _decode_opc(DisasContext * ctx)
> gen_op_shlr16_Rn(REG(B11_8));
> return;
> case 0x401b: /* tas.b @Rn */
> - gen_op_tasb_rN(REG(B11_8));
> + gen_op_tasb_Rn(ctx, REG(B11_8));
> return;
> case 0xf00d: /* fsts FPUL,FRn - FPSCR: Nothing */
> gen_op_movl_fpul_FT0();
This proposed fix for an open bug is about to be forgotten again. Can
anyone with SH4 experience comment on it?
Jan
signature.asc
Description: OpenPGP digital signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- [Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (Jan Kiszka).,
Jan Kiszka <=