diff mbox series

[v2,18/22] semihosting/arm-compat: Clean up local variable shadowing

Message ID 20230904161235.84651-19-philmd@linaro.org
State New
Headers show
Series (few more) Steps towards enabling -Wshadow | expand

Commit Message

Philippe Mathieu-Daudé Sept. 4, 2023, 4:12 p.m. UTC
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(-)

Comments

Peter Maydell Sept. 8, 2023, 12:36 p.m. UTC | #1
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
Philippe Mathieu-Daudé Oct. 4, 2023, 9:41 a.m. UTC | #2
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 mbox series

Patch

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);