| Message ID | 20210505160344.1394843-1-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/m68k: Remove unused variable in ABCD/SBCD memory opcodes | expand |
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
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
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?
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);
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(-)