[PATCH] target/m68k: Remove unused variable in ABCD/SBCD memory opcodes

[PATCH] target/m68k: Remove unused variable in ABCD/SBCD memory opcodes

[Philippe Mathieu-Daudé]

The ABCD / SBCD memory opcodes (introduced in commit fb5543d8200)
don't use their "addr" variable.

Remove the unused variable and pass a NULL argument instead to
gen_ea_mode(). This fixes warnings generated when building with
CFLAGS=-O3 (using GCC 10.2.1 20201125):

  target/m68k/translate.c: In function ‘disas_sbcd_mem’:
  target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    897 |             delay_set_areg(s, reg0, tmp, false);
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  target/m68k/translate.c:1866:21: note: ‘addr’ was declared here
   1866 |     TCGv src, dest, addr;
        |                     ^~~~

  target/m68k/translate.c: In function ‘disas_abcd_mem’:
  target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    897 |             delay_set_areg(s, reg0, tmp, false);
        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  target/m68k/translate.c:1829:21: note: ‘addr’ was declared here
   1829 |     TCGv src, dest, addr;
        |                     ^~~~

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 200018ae6a6..5cdd026a4b2 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1826,7 +1826,7 @@ DISAS_INSN(abcd_reg)
 
 DISAS_INSN(abcd_mem)
 {
-    TCGv src, dest, addr;
+    TCGv src, dest;
 
     gen_flush_flags(s); /* !Z is sticky */
 
@@ -1835,11 +1835,11 @@ DISAS_INSN(abcd_mem)
     src = gen_ea_mode(env, s, 4, REG(insn, 0), OS_BYTE,
                       NULL_QREG, NULL, EA_LOADU, IS_USER(s));
     dest = gen_ea_mode(env, s, 4, REG(insn, 9), OS_BYTE,
-                       NULL_QREG, &addr, EA_LOADU, IS_USER(s));
+                       NULL_QREG, NULL, EA_LOADU, IS_USER(s));
 
     bcd_add(dest, src);
 
-    gen_ea_mode(env, s, 4, REG(insn, 9), OS_BYTE, dest, &addr,
+    gen_ea_mode(env, s, 4, REG(insn, 9), OS_BYTE, dest, NULL,
                 EA_STORE, IS_USER(s));
 
     bcd_flags(dest);
@@ -1863,7 +1863,7 @@ DISAS_INSN(sbcd_reg)
 
 DISAS_INSN(sbcd_mem)
 {
-    TCGv src, dest, addr;
+    TCGv src, dest;
 
     gen_flush_flags(s); /* !Z is sticky */
 
@@ -1872,11 +1872,11 @@ DISAS_INSN(sbcd_mem)
     src = gen_ea_mode(env, s, 4, REG(insn, 0), OS_BYTE,
                       NULL_QREG, NULL, EA_LOADU, IS_USER(s));
     dest = gen_ea_mode(env, s, 4, REG(insn, 9), OS_BYTE,
-                       NULL_QREG, &addr, EA_LOADU, IS_USER(s));
+                       NULL_QREG, NULL, EA_LOADU, IS_USER(s));
 
     bcd_sub(dest, src);
 
-    gen_ea_mode(env, s, 4, REG(insn, 9), OS_BYTE, dest, &addr,
+    gen_ea_mode(env, s, 4, REG(insn, 9), OS_BYTE, dest, NULL,
                 EA_STORE, IS_USER(s));
 
     bcd_flags(dest);
-- 
2.26.3

Re: [PATCH] target/m68k: Remove unused variable in ABCD/SBCD memory opcodes

[Laurent Vivier]

Le 05/05/2021 à 18:03, Philippe Mathieu-Daudé a écrit :
> The ABCD / SBCD memory opcodes (introduced in commit fb5543d8200)
> don't use their "addr" variable.
> 
> Remove the unused variable and pass a NULL argument instead to
> gen_ea_mode(). This fixes warnings generated when building with
> CFLAGS=-O3 (using GCC 10.2.1 20201125):
> 
>   target/m68k/translate.c: In function ‘disas_sbcd_mem’:
>   target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     897 |             delay_set_areg(s, reg0, tmp, false);
>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   target/m68k/translate.c:1866:21: note: ‘addr’ was declared here
>    1866 |     TCGv src, dest, addr;
>         |                     ^~~~
> 
>   target/m68k/translate.c: In function ‘disas_abcd_mem’:
>   target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     897 |             delay_set_areg(s, reg0, tmp, false);
>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   target/m68k/translate.c:1829:21: note: ‘addr’ was declared here
>    1829 |     TCGv src, dest, addr;
>         |                     ^~~~
> 

It's really strange because ABCD and SBCD support indirect predecrement (mode 4, "-(Ay),-(Ax)"), and
if you look into gen_ea_mode() &addr (addrp) is used with mode 4, it is initialized on EA_LOADU to
be reused on EA_STORE.

The bug is somewhere else...

Thanks,
Laurent

Re: [PATCH] target/m68k: Remove unused variable in ABCD/SBCD memory opcodes

[Laurent Vivier]

Le 05/05/2021 à 18:43, Laurent Vivier a écrit :
> Le 05/05/2021 à 18:03, Philippe Mathieu-Daudé a écrit :
>> The ABCD / SBCD memory opcodes (introduced in commit fb5543d8200)
>> don't use their "addr" variable.
>>
>> Remove the unused variable and pass a NULL argument instead to
>> gen_ea_mode(). This fixes warnings generated when building with
>> CFLAGS=-O3 (using GCC 10.2.1 20201125):
>>
>>   target/m68k/translate.c: In function ‘disas_sbcd_mem’:
>>   target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>     897 |             delay_set_areg(s, reg0, tmp, false);
>>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   target/m68k/translate.c:1866:21: note: ‘addr’ was declared here
>>    1866 |     TCGv src, dest, addr;
>>         |                     ^~~~
>>
>>   target/m68k/translate.c: In function ‘disas_abcd_mem’:
>>   target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>     897 |             delay_set_areg(s, reg0, tmp, false);
>>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   target/m68k/translate.c:1829:21: note: ‘addr’ was declared here
>>    1829 |     TCGv src, dest, addr;
>>         |                     ^~~~
>>
> 
> It's really strange because ABCD and SBCD support indirect predecrement (mode 4, "-(Ay),-(Ax)"), and
> if you look into gen_ea_mode() &addr (addrp) is used with mode 4, it is initialized on EA_LOADU to
> be reused on EA_STORE.
> 
> The bug is somewhere else...
> 

I think I see what is the problem: as the mode is indirect pre-decrement, the register doesn't need
to be updated and thus the addr is not needed.

But if we replace addrp by NULL, gen_lea_mode() will be called twice and the register will be
decremented twice (on load and store, rather than only on load).

Thanks,
Laurent

Re: [PATCH] target/m68k: Remove unused variable in ABCD/SBCD memory opcodes

[Philippe Mathieu-Daudé]

On 5/6/21 12:29 PM, Laurent Vivier wrote:
> Le 05/05/2021 à 18:43, Laurent Vivier a écrit :
>> Le 05/05/2021 à 18:03, Philippe Mathieu-Daudé a écrit :
>>> The ABCD / SBCD memory opcodes (introduced in commit fb5543d8200)
>>> don't use their "addr" variable.
>>>
>>> Remove the unused variable and pass a NULL argument instead to
>>> gen_ea_mode(). This fixes warnings generated when building with
>>> CFLAGS=-O3 (using GCC 10.2.1 20201125):
>>>
>>>   target/m68k/translate.c: In function ‘disas_sbcd_mem’:
>>>   target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>     897 |             delay_set_areg(s, reg0, tmp, false);
>>>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   target/m68k/translate.c:1866:21: note: ‘addr’ was declared here
>>>    1866 |     TCGv src, dest, addr;
>>>         |                     ^~~~
>>>
>>>   target/m68k/translate.c: In function ‘disas_abcd_mem’:
>>>   target/m68k/translate.c:897:13: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>>     897 |             delay_set_areg(s, reg0, tmp, false);
>>>         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   target/m68k/translate.c:1829:21: note: ‘addr’ was declared here
>>>    1829 |     TCGv src, dest, addr;
>>>         |                     ^~~~
>>>
>>
>> It's really strange because ABCD and SBCD support indirect predecrement (mode 4, "-(Ay),-(Ax)"), and
>> if you look into gen_ea_mode() &addr (addrp) is used with mode 4, it is initialized on EA_LOADU to
>> be reused on EA_STORE.
>>
>> The bug is somewhere else...
>>
> 
> I think I see what is the problem: as the mode is indirect pre-decrement, the register doesn't need
> to be updated and thus the addr is not needed.
> 
> But if we replace addrp by NULL, gen_lea_mode() will be called twice and the register will be
> decremented twice (on load and store, rather than only on load).

Ah OK I see. Well, I guess I'll let you have a look, you clearly have
better understanding :P Would it help if I fill a GitLab issue?