* [Qemu-devel] [PATCH] target-lm32: fix style issue
@ 2016-10-12 16:23 Michael Walle
2016-10-12 16:29 ` Thomas Huth
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Michael Walle @ 2016-10-12 16:23 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Thomas Huth, Michael Walle
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;
--
2.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 16:23 [Qemu-devel] [PATCH] target-lm32: fix style issue Michael Walle
@ 2016-10-12 16:29 ` Thomas Huth
2016-10-12 16:35 ` Peter Maydell
2016-10-15 12:35 ` Michael Tokarev
2 siblings, 0 replies; 8+ messages in thread
From: Thomas Huth @ 2016-10-12 16:29 UTC (permalink / raw)
To: Michael Walle, qemu-devel; +Cc: qemu-trivial, Thomas Huth
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
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>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 16:23 [Qemu-devel] [PATCH] target-lm32: fix style issue Michael Walle
2016-10-12 16:29 ` Thomas Huth
@ 2016-10-12 16:35 ` Peter Maydell
2016-10-12 16:42 ` Michael Walle
2016-10-12 17:11 ` Michael Walle
2016-10-15 12:35 ` Michael Tokarev
2 siblings, 2 replies; 8+ messages in thread
From: Peter Maydell @ 2016-10-12 16:35 UTC (permalink / raw)
To: Michael Walle; +Cc: QEMU Developers, QEMU Trivial, Thomas Huth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 16:35 ` Peter Maydell
@ 2016-10-12 16:42 ` Michael Walle
2016-10-12 17:12 ` Peter Maydell
2016-10-12 17:11 ` Michael Walle
1 sibling, 1 reply; 8+ messages in thread
From: Michael Walle @ 2016-10-12 16:42 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, QEMU Trivial, Thomas Huth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 16:35 ` Peter Maydell
2016-10-12 16:42 ` Michael Walle
@ 2016-10-12 17:11 ` Michael Walle
2016-10-12 17:21 ` Peter Maydell
1 sibling, 1 reply; 8+ messages in thread
From: Michael Walle @ 2016-10-12 17:11 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, QEMU Trivial, Thomas Huth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 16:42 ` Michael Walle
@ 2016-10-12 17:12 ` Peter Maydell
0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2016-10-12 17:12 UTC (permalink / raw)
To: Michael Walle; +Cc: QEMU Developers, QEMU Trivial, Thomas Huth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 17:11 ` Michael Walle
@ 2016-10-12 17:21 ` Peter Maydell
0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2016-10-12 17:21 UTC (permalink / raw)
To: Michael Walle; +Cc: QEMU Developers, QEMU Trivial, Thomas Huth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] target-lm32: fix style issue
2016-10-12 16:23 [Qemu-devel] [PATCH] target-lm32: fix style issue Michael Walle
2016-10-12 16:29 ` Thomas Huth
2016-10-12 16:35 ` Peter Maydell
@ 2016-10-15 12:35 ` Michael Tokarev
2 siblings, 0 replies; 8+ messages in thread
From: Michael Tokarev @ 2016-10-15 12:35 UTC (permalink / raw)
To: Michael Walle, qemu-devel; +Cc: qemu-trivial, Thomas Huth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-15 12:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-12 16:23 [Qemu-devel] [PATCH] target-lm32: fix style issue Michael Walle
2016-10-12 16:29 ` Thomas Huth
2016-10-12 16:35 ` Peter Maydell
2016-10-12 16:42 ` Michael Walle
2016-10-12 17:12 ` Peter Maydell
2016-10-12 17:11 ` Michael Walle
2016-10-12 17:21 ` Peter Maydell
2016-10-15 12:35 ` Michael Tokarev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.