| Message ID | 1476289396-10780-1-git-send-email-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
On 12.10.2016 18:23, Michael Walle wrote: > Both branches of the ternary operator have the same expressions. Drop the > operator. > > This fixes: https://bugs.launchpad.net/qemu/+bug/1414293 > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > target-lm32/translate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-lm32/translate.c b/target-lm32/translate.c > index 2d8caeb..534c17c 100644 > --- a/target-lm32/translate.c > +++ b/target-lm32/translate.c > @@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc) > static inline void gen_compare(DisasContext *dc, int cond) > { > int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1; > - int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0; > + int rY = dc->r0; > int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1; > int i; Reviewed-by: Thomas Huth <huth@tuxfamily.org>
On 12 October 2016 at 17:23, Michael Walle <michael@walle.cc> wrote: > Both branches of the ternary operator have the same expressions. Drop the > operator. > > This fixes: https://bugs.launchpad.net/qemu/+bug/1414293 > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > target-lm32/translate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-lm32/translate.c b/target-lm32/translate.c > index 2d8caeb..534c17c 100644 > --- a/target-lm32/translate.c > +++ b/target-lm32/translate.c > @@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc) > static inline void gen_compare(DisasContext *dc, int cond) > { > int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1; > - int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0; > + int rY = dc->r0; > int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1; > int i; This checks against the processor reference manual, so: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> but I noticed while doing the review that our LOG_DIS is wrong for the compare-immediates: LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1, sign_extend(dc->imm16, 16)); but the processor reference manual says cmpei's mnemonic should have dc->r1 first and dc->r0 second. (Similarly for the logging for the other immediate compares.) thanks -- PMM
Am 2016-10-12 18:35, schrieb Peter Maydell: > but I noticed while doing the review that our LOG_DIS > is wrong for the compare-immediates: > > LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1, > sign_extend(dc->imm16, 16)); > > but the processor reference manual says cmpei's mnemonic > should have dc->r1 first and dc->r0 second. > > (Similarly for the logging for the other immediate compares.) Argh, you're eyes are too good ;) I'll have a look. -michael
Am 2016-10-12 18:35, schrieb Peter Maydell: > On 12 October 2016 at 17:23, Michael Walle <michael@walle.cc> wrote: >> Both branches of the ternary operator have the same expressions. Drop >> the >> operator. >> >> This fixes: https://bugs.launchpad.net/qemu/+bug/1414293 >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> target-lm32/translate.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target-lm32/translate.c b/target-lm32/translate.c >> index 2d8caeb..534c17c 100644 >> --- a/target-lm32/translate.c >> +++ b/target-lm32/translate.c >> @@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc) >> static inline void gen_compare(DisasContext *dc, int cond) >> { >> int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1; >> - int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0; >> + int rY = dc->r0; >> int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1; >> int i; > > This checks against the processor reference manual, so: > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > but I noticed while doing the review that our LOG_DIS > is wrong for the compare-immediates: > > LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1, > sign_extend(dc->imm16, 16)); > > but the processor reference manual says cmpei's mnemonic > should have dc->r1 first and dc->r0 second. Hi Peter, can I drop the DISAS_LM32 macro and just always enable the qemu_log_mask(CPU_LOG_TB_IN_ASM)? Looking at other CPUs this is sometimes a (debug) compile switch (eg ppc) and sometimes its always enabled (tilegx). -michael
On 12 October 2016 at 17:42, Michael Walle <michael@walle.cc> wrote: > Am 2016-10-12 18:35, schrieb Peter Maydell: >> >> but I noticed while doing the review that our LOG_DIS >> is wrong for the compare-immediates: >> >> LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1, >> sign_extend(dc->imm16, 16)); >> >> but the processor reference manual says cmpei's mnemonic >> should have dc->r1 first and dc->r0 second. >> >> (Similarly for the logging for the other immediate compares.) > > > Argh, you're eyes are too good ;) I'll have a look. If you're looking at lm32 bugs in general, you might also be interested in the one coverity report for lm32, which is that in hw/display/milkymist-tmu2.c this code from tmu2_start() fb_len = 2*s->regs[R_TEXHRES]*s->regs[R_TEXVRES]; is reported as a potential overflow, because the s->regs[] fields are 32 bits and so the multiplies are done as 32*32 (truncating) but fb_len is 64 bit. Changing the 2 to 2ULL is probably the simplest fix... thanks -- PMM
On 12 October 2016 at 18:11, Michael Walle <michael@walle.cc> wrote: > Am 2016-10-12 18:35, schrieb Peter Maydell: >> but I noticed while doing the review that our LOG_DIS >> is wrong for the compare-immediates: >> >> LOG_DIS("cmpei r%d, r%d, %d\n", dc->r0, dc->r1, >> sign_extend(dc->imm16, 16)); >> >> but the processor reference manual says cmpei's mnemonic >> should have dc->r1 first and dc->r0 second. > can I drop the DISAS_LM32 macro and just always enable the > qemu_log_mask(CPU_LOG_TB_IN_ASM)? Looking at other CPUs this is sometimes a > (debug) compile switch (eg ppc) and sometimes its always enabled (tilegx). tilegx unconditionally does this logging because there's no tilegx disassembler in disas/, and so printing log statements in the decoder is a dubious but low effort way to achieve the desired effect (that -d in_asm prints the guest disassembly). ppc on the other hand has a proper disassembler in disas/, and so the LOG_DISAS() stuff is just debug-printf stuff for the purposes of tracking down bugs in the decoder, and like most of the debug-printf macros in devices it is not compiled in by default (you turn it on by hand if you have a bug it might help with), and exactly what is logged is very much ad-hoc. But lm32 has a disassembler in disas, so it doesn't need to emit this stuff for -d in_asm to work, and indeed shouldn't emit it by default, otherwise all the disassembly would be printed twice. You can see this bug if you do: ./build/x86-all/lm32-softmmu/qemu-system-lm32 -d in_asm -D /tmp/lm32.log -M milkymist -serial stdio -kernel ~/test-images/milkymist/flickernoise as the resulting log looks like: 40000000: 98000000 xor r0, r0, r0 40000004: d0000000 wcsr r0, 0 0x40000000: 98 00 00 00 xor r0, r0, r0 0x40000004: d0 00 00 00 wcsr ie, r0 isize=8 osize=11 40000008: d0200000 wcsr r0, 1 0x40000008: d0 20 00 00 wcsr im, r0 isize=4 osize=9 4000000c: 78014000 mvhi r1, 16384 40000010: 38210000 ori r1, r1, 0 40000014: d0e10000 wcsr r1, 7 40000018: e000003a bi 232 0x4000000c: 78 01 40 00 orhi r1, r0, 0x4000 0x40000010: 38 21 00 00 ori r1, r1, 0x0 0x40000014: d0 e1 00 00 wcsr eba, r1 0x40000018: e0 00 00 3a bi 40000100 mixing the disassembly from the disassembler with the debug output from the translate.c code in a somewhat confusing way. thanks -- PMM
12.10.2016 19:23, Michael Walle wrote: > Both branches of the ternary operator have the same expressions. Drop the > operator. Applied to -trivial, thank you! /mjt
diff --git a/target-lm32/translate.c b/target-lm32/translate.c index 2d8caeb..534c17c 100644 --- a/target-lm32/translate.c +++ b/target-lm32/translate.c @@ -343,7 +343,7 @@ static void dec_calli(DisasContext *dc) static inline void gen_compare(DisasContext *dc, int cond) { int rX = (dc->format == OP_FMT_RR) ? dc->r2 : dc->r1; - int rY = (dc->format == OP_FMT_RR) ? dc->r0 : dc->r0; + int rY = dc->r0; int rZ = (dc->format == OP_FMT_RR) ? dc->r1 : -1; int i;
Both branches of the ternary operator have the same expressions. Drop the operator. This fixes: https://bugs.launchpad.net/qemu/+bug/1414293 Signed-off-by: Michael Walle <michael@walle.cc> --- target-lm32/translate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)