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?