| Message ID | 20230904161235.84651-19-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | (few more) Steps towards enabling -Wshadow | expand |
On Mon, 4 Sept 2023 at 17:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Fix: > > semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’: > semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local] > 379 | int ret, err = 0; > | ^~~ > semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here > 370 | uint32_t ret; > | ^~~ > semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local] > 682 | abi_ulong ret; > | ^~~ > semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here > 370 | int ret; > | ^~~ > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > semihosting/arm-compat-semi.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) If I'm reading the code correctly, the top level 'ret' variable is only used by the SYS_EXIT case currently. So rather than changing the type of it I think it would be better to remove it and have a variable at tighter scope for SYS_EXIT. I think that's easier to read than this kind of "single variable used for multiple purposes at different places within a long function". > diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c > index 564fe17f75..85852a15b8 100644 > --- a/semihosting/arm-compat-semi.c > +++ b/semihosting/arm-compat-semi.c > @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs) > target_ulong ul_ret; > char * s; > int nr; > - uint32_t ret; > + int ret; > int64_t elapsed; > > nr = common_semi_arg(cs, 0) & 0xffffffffU; > @@ -376,7 +376,7 @@ void do_common_semihosting(CPUState *cs) > switch (nr) { > case TARGET_SYS_OPEN: > { > - int ret, err = 0; > + int err = 0; > int hostfd; > > GET_ARG(0); > @@ -679,14 +679,11 @@ void do_common_semihosting(CPUState *cs) > * allocate it using sbrk. > */ > if (!ts->heap_limit) { > - abi_ulong ret; > - > ts->heap_base = do_brk(0); > limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE; > /* Try a big heap, and reduce the size if that fails. */ > for (;;) { > - ret = do_brk(limit); > - if (ret >= limit) { > + if (do_brk(limit) >= limit) { > break; > } > limit = (ts->heap_base >> 1) + (limit >> 1); > - thanks -- PMM
Hi Peter, Alex, On 8/9/23 14:36, Peter Maydell wrote: > On Mon, 4 Sept 2023 at 17:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Fix: >> >> semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’: >> semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local] >> 379 | int ret, err = 0; >> | ^~~ >> semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here >> 370 | uint32_t ret; >> | ^~~ >> semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local] >> 682 | abi_ulong ret; >> | ^~~ >> semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here >> 370 | int ret; >> | ^~~ >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> semihosting/arm-compat-semi.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) > > If I'm reading the code correctly, the top level 'ret' variable > is only used by the SYS_EXIT case currently. So rather than > changing the type of it I think it would be better to remove > it and have a variable at tighter scope for SYS_EXIT. I > think that's easier to read than this kind of "single variable > used for multiple purposes at different places within a > long function". Yes you are right. Now looking at it, we currently use a uint32_t type, but per https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst#652entry-64-bit: In particular, if field 1 is ADP_Stopped_ApplicationExit then field 2 is an exit status code, as passed to the C standard library exit() function. A simulator receiving this request must notify a connected debugger, if present, and then exit with the specified status. exit() is declared as: LIBRARY Standard C Library (libc, -lc) SYNOPSIS void exit(int status); So it expects a signed status code, but we convert it to unsigned... Alex, shouldn't we use a 'int' type here instead of 'uint32_t'? >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c >> index 564fe17f75..85852a15b8 100644 >> --- a/semihosting/arm-compat-semi.c >> +++ b/semihosting/arm-compat-semi.c >> @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs) >> target_ulong ul_ret; >> char * s; >> int nr; >> - uint32_t ret; >> + int ret; >> int64_t elapsed;
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c index 564fe17f75..85852a15b8 100644 --- a/semihosting/arm-compat-semi.c +++ b/semihosting/arm-compat-semi.c @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs) target_ulong ul_ret; char * s; int nr; - uint32_t ret; + int ret; int64_t elapsed; nr = common_semi_arg(cs, 0) & 0xffffffffU; @@ -376,7 +376,7 @@ void do_common_semihosting(CPUState *cs) switch (nr) { case TARGET_SYS_OPEN: { - int ret, err = 0; + int err = 0; int hostfd; GET_ARG(0); @@ -679,14 +679,11 @@ void do_common_semihosting(CPUState *cs) * allocate it using sbrk. */ if (!ts->heap_limit) { - abi_ulong ret; - ts->heap_base = do_brk(0); limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE; /* Try a big heap, and reduce the size if that fails. */ for (;;) { - ret = do_brk(limit); - if (ret >= limit) { + if (do_brk(limit) >= limit) { break; } limit = (ts->heap_base >> 1) + (limit >> 1);
Fix: semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’: semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local] 379 | int ret, err = 0; | ^~~ semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here 370 | uint32_t ret; | ^~~ semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local] 682 | abi_ulong ret; | ^~~ semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here 370 | int ret; | ^~~ Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- semihosting/arm-compat-semi.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)