* [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka). @ 2008-07-16 11:31 Andrzej Zaborowski 2008-07-16 11:36 ` Laurent Desnogues 2008-07-16 12:04 ` [Qemu-devel] " Andreas Schwab 0 siblings, 2 replies; 8+ messages in thread From: Andrzej Zaborowski @ 2008-07-16 11:31 UTC (permalink / raw) To: qemu-devel Revision: 4875 http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=4875 Author: balrog Date: 2008-07-16 11:31:11 +0000 (Wed, 16 Jul 2008) Log Message: ----------- Remove unintended dereference, kills a warning (Jan Kiszka). Modified Paths: -------------- trunk/target-sh4/op.c Modified: trunk/target-sh4/op.c =================================================================== --- trunk/target-sh4/op.c 2008-07-16 04:45:12 UTC (rev 4874) +++ trunk/target-sh4/op.c 2008-07-16 11:31:11 UTC (rev 4875) @@ -594,8 +594,8 @@ 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; RETURN(); } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-16 11:31 [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka) Andrzej Zaborowski @ 2008-07-16 11:36 ` Laurent Desnogues 2008-07-16 12:10 ` [Qemu-devel] " Jan Kiszka 2008-07-16 12:04 ` [Qemu-devel] " Andreas Schwab 1 sibling, 1 reply; 8+ messages in thread From: Laurent Desnogues @ 2008-07-16 11:36 UTC (permalink / raw) To: qemu-devel On Wed, Jul 16, 2008 at 1:31 PM, Andrzej Zaborowski <balrogg@gmail.com> wrote: > > Modified: trunk/target-sh4/op.c > =================================================================== > --- trunk/target-sh4/op.c 2008-07-16 04:45:12 UTC (rev 4874) > +++ trunk/target-sh4/op.c 2008-07-16 11:31:11 UTC (rev 4875) > @@ -594,8 +594,8 @@ > > 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); Guess that should be a single '&'. > + *(int8_t *) &env->gregs[PARAM1] |= 0x80; > RETURN(); > } Laurent ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-16 11:36 ` Laurent Desnogues @ 2008-07-16 12:10 ` Jan Kiszka 0 siblings, 0 replies; 8+ messages in thread From: Jan Kiszka @ 2008-07-16 12:10 UTC (permalink / raw) To: qemu-devel [-- Attachment #1: Type: text/plain, Size: 672 bytes --] Laurent Desnogues wrote: > On Wed, Jul 16, 2008 at 1:31 PM, Andrzej Zaborowski <balrogg@gmail.com> wrote: >> Modified: trunk/target-sh4/op.c >> =================================================================== >> --- trunk/target-sh4/op.c 2008-07-16 04:45:12 UTC (rev 4874) >> +++ trunk/target-sh4/op.c 2008-07-16 11:31:11 UTC (rev 4875) >> @@ -594,8 +594,8 @@ >> >> 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); > > Guess that should be a single '&'. Oh, ooh. Yes, please fix. Thanks, Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-16 11:31 [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka) Andrzej Zaborowski 2008-07-16 11:36 ` Laurent Desnogues @ 2008-07-16 12:04 ` Andreas Schwab 2008-07-16 12:30 ` Laurent Desnogues 1 sibling, 1 reply; 8+ messages in thread From: Andreas Schwab @ 2008-07-16 12:04 UTC (permalink / raw) To: qemu-devel Andrzej Zaborowski <balrogg@gmail.com> 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). Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-16 12:04 ` [Qemu-devel] " Andreas Schwab @ 2008-07-16 12:30 ` Laurent Desnogues 2008-07-16 12:35 ` [Qemu-devel] " Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: Laurent Desnogues @ 2008-07-16 12:30 UTC (permalink / raw) To: qemu-devel On Wed, Jul 16, 2008 at 2:04 PM, Andreas Schwab <schwab@suse.de> wrote: > Andrzej Zaborowski <balrogg@gmail.com> 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. Laurent ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-16 12:30 ` Laurent Desnogues @ 2008-07-16 12:35 ` Jan Kiszka 2008-07-19 6:24 ` andrzej zaborowski 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2008-07-16 12:35 UTC (permalink / raw) To: qemu-devel Laurent Desnogues wrote: > On Wed, Jul 16, 2008 at 2:04 PM, Andreas Schwab <schwab@suse.de> wrote: >> Andrzej Zaborowski <balrogg@gmail.com> 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? Index: qemu/target-sh4/op.c =================================================================== --- qemu.orig/target-sh4/op.c +++ qemu/target-sh4/op.c @@ -594,8 +594,9 @@ void OPPROTO op_shlr16_Rn(void) void OPPROTO op_tasb_rN(void) { - cond_t(*(int8_t *) env->gregs[PARAM1] == 0); - *(int8_t *) env->gregs[PARAM1] |= 0x80; + uint8_t val = ldub(env->gregs[PARAM1]); + cond_t(val == 0); + stb(env->gregs[PARAM1], val | 0x80); RETURN(); } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-16 12:35 ` [Qemu-devel] " Jan Kiszka @ 2008-07-19 6:24 ` andrzej zaborowski 2008-08-18 7:13 ` Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: andrzej zaborowski @ 2008-07-19 6:24 UTC (permalink / raw) To: qemu-devel 2008/7/16 Jan Kiszka <jan.kiszka@web.de>: > Laurent Desnogues wrote: >> On Wed, Jul 16, 2008 at 2:04 PM, Andreas Schwab <schwab@suse.de> wrote: >>> Andrzej Zaborowski <balrogg@gmail.com> 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(); ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] Re: [4875] Remove unintended dereference, kills a warning (Jan Kiszka). 2008-07-19 6:24 ` andrzej zaborowski @ 2008-08-18 7:13 ` Jan Kiszka 0 siblings, 0 replies; 8+ messages in thread From: Jan Kiszka @ 2008-08-18 7:13 UTC (permalink / raw) To: qemu-devel [-- Attachment #1: Type: text/plain, Size: 4429 bytes --] andrzej zaborowski wrote: > 2008/7/16 Jan Kiszka <jan.kiszka@web.de>: >> Laurent Desnogues wrote: >>> On Wed, Jul 16, 2008 at 2:04 PM, Andreas Schwab <schwab@suse.de> wrote: >>>> Andrzej Zaborowski <balrogg@gmail.com> 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-08-18 7:14 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-16 11:31 [Qemu-devel] [4875] Remove unintended dereference, kills a warning (Jan Kiszka) Andrzej Zaborowski 2008-07-16 11:36 ` Laurent Desnogues 2008-07-16 12:10 ` [Qemu-devel] " Jan Kiszka 2008-07-16 12:04 ` [Qemu-devel] " Andreas Schwab 2008-07-16 12:30 ` Laurent Desnogues 2008-07-16 12:35 ` [Qemu-devel] " Jan Kiszka 2008-07-19 6:24 ` andrzej zaborowski 2008-08-18 7:13 ` Jan Kiszka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).