device_tree: Fix compiler error

[PATCH] device_tree: Fix compiler error

Posted by Stefan Weil 2 years, 6 months ago

A build with gcc (Debian 10.2.1-6) 10.2.1 20210110 fails:

../../../softmmu/device_tree.c: In function ‘qemu_fdt_add_path’:
../../../softmmu/device_tree.c:560:18: error: ‘retval’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  560 |     int namelen, retval;
      |                  ^~~~~~

This is not a real error, but the compiler can be satisfied with a small change.

Fixes: b863f0b75852 ("device_tree: Add qemu_fdt_add_path")
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 softmmu/device_tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
index 3965c834ca..9e96f5ecd5 100644
--- a/softmmu/device_tree.c
+++ b/softmmu/device_tree.c
@@ -564,7 +564,7 @@ int qemu_fdt_add_path(void *fdt, const char *path)
         return -1;
     }
 
-    while (p) {
+    do {
         name = p + 1;
         p = strchr(name, '/');
         namelen = p != NULL ? p - name : strlen(name);
@@ -584,7 +584,7 @@ int qemu_fdt_add_path(void *fdt, const char *path)
         }
 
         parent = retval;
-    }
+    } while (p);
 
     return retval;
 }
-- 
2.30.2

Re: [PATCH] device_tree: Fix compiler error

Posted by Alistair Francis 2 years, 6 months ago

On Tue, Nov 9, 2021 at 6:08 AM Stefan Weil <sw@weilnetz.de> wrote:
>
> A build with gcc (Debian 10.2.1-6) 10.2.1 20210110 fails:
>
> ../../../softmmu/device_tree.c: In function ‘qemu_fdt_add_path’:
> ../../../softmmu/device_tree.c:560:18: error: ‘retval’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   560 |     int namelen, retval;
>       |                  ^~~~~~
>
> This is not a real error, but the compiler can be satisfied with a small change.

Why don't we just initalise retval?

Alistair

>
> Fixes: b863f0b75852 ("device_tree: Add qemu_fdt_add_path")
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  softmmu/device_tree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 3965c834ca..9e96f5ecd5 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -564,7 +564,7 @@ int qemu_fdt_add_path(void *fdt, const char *path)
>          return -1;
>      }
>
> -    while (p) {
> +    do {
>          name = p + 1;
>          p = strchr(name, '/');
>          namelen = p != NULL ? p - name : strlen(name);
> @@ -584,7 +584,7 @@ int qemu_fdt_add_path(void *fdt, const char *path)
>          }
>
>          parent = retval;
> -    }
> +    } while (p);
>
>      return retval;
>  }
> --
> 2.30.2
>
>

Re: [PATCH] device_tree: Fix compiler error

Posted by Stefan Weil 2 years, 6 months ago

Am 08.11.21 um 23:43 schrieb Alistair Francis:

> On Tue, Nov 9, 2021 at 6:08 AM Stefan Weil <sw@weilnetz.de> wrote:
>> A build with gcc (Debian 10.2.1-6) 10.2.1 20210110 fails:
>>
>> ../../../softmmu/device_tree.c: In function ‘qemu_fdt_add_path’:
>> ../../../softmmu/device_tree.c:560:18: error: ‘retval’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    560 |     int namelen, retval;
>>        |                  ^~~~~~
>>
>> This is not a real error, but the compiler can be satisfied with a small change.
> Why don't we just initalise retval?
>
> Alistair

retval is already set in the do ... while loop and was also set in the 
former while loop.

If we set it in the declaration (int retval = 0), a clever compiler 
might complain that we assign a value which is never used.

And changing from the while loop to the do ... while loop also avoids 
one compare statement, so depending on the compiler optimizations could 
make the code a little bit faster.

Stefan

Re: [PATCH] device_tree: Fix compiler error

Posted by Richard Henderson 2 years, 6 months ago

On 11/8/21 9:07 PM, Stefan Weil wrote:
> A build with gcc (Debian 10.2.1-6) 10.2.1 20210110 fails:
> 
> ../../../softmmu/device_tree.c: In function ‘qemu_fdt_add_path’:
> ../../../softmmu/device_tree.c:560:18: error: ‘retval’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    560 |     int namelen, retval;
>        |                  ^~~~~~
> 
> This is not a real error, but the compiler can be satisfied with a small change.
> 
> Fixes: b863f0b75852 ("device_tree: Add qemu_fdt_add_path")
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Though I think there's a good deal that could be cleaned up about this function:

(1a) Remove the unused return value?
      The single use does not check the return.

(1b) Don't attempt to return a node, merely a success/failure code.
      Certainly the local documentation here could be improved...

(1c) Return parent; make retval local to the loop.

(2) Merge p and path; there's no point retaining the unmodified parameter.

(3) Move name and namelen inside the loop.


r~

Re: [PATCH] device_tree: Fix compiler error

Posted by Michal Prívozník 2 years, 6 months ago

On 11/9/21 9:38 AM, Richard Henderson wrote:
> On 11/8/21 9:07 PM, Stefan Weil wrote:
>> A build with gcc (Debian 10.2.1-6) 10.2.1 20210110 fails:
>>
>> ../../../softmmu/device_tree.c: In function ‘qemu_fdt_add_path’:
>> ../../../softmmu/device_tree.c:560:18: error: ‘retval’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>    560 |     int namelen, retval;
>>        |                  ^~~~~~
>>
>> This is not a real error, but the compiler can be satisfied with a
>> small change.
>>
>> Fixes: b863f0b75852 ("device_tree: Add qemu_fdt_add_path")
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Though I think there's a good deal that could be cleaned up about this
> function:
> 
> (1a) Remove the unused return value?
>      The single use does not check the return.
> 
> (1b) Don't attempt to return a node, merely a success/failure code.
>      Certainly the local documentation here could be improved...
> 
> (1c) Return parent; make retval local to the loop.
> 
> (2) Merge p and path; there's no point retaining the unmodified parameter.
> 
> (3) Move name and namelen inside the loop.


(4) swap if() bodies?

if (retval == -FDT_ERR_NOTFOUND) {
} else if (retval < 0) {
}

Michal

Re: [PATCH] device_tree: Fix compiler error

Posted by Laurent Vivier 2 years, 5 months ago

Le 08/11/2021 à 21:07, Stefan Weil a écrit :
> A build with gcc (Debian 10.2.1-6) 10.2.1 20210110 fails:
> 
> ../../../softmmu/device_tree.c: In function ‘qemu_fdt_add_path’:
> ../../../softmmu/device_tree.c:560:18: error: ‘retval’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    560 |     int namelen, retval;
>        |                  ^~~~~~
> 
> This is not a real error, but the compiler can be satisfied with a small change.
> 
> Fixes: b863f0b75852 ("device_tree: Add qemu_fdt_add_path")
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>   softmmu/device_tree.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> index 3965c834ca..9e96f5ecd5 100644
> --- a/softmmu/device_tree.c
> +++ b/softmmu/device_tree.c
> @@ -564,7 +564,7 @@ int qemu_fdt_add_path(void *fdt, const char *path)
>           return -1;
>       }
>   
> -    while (p) {
> +    do {
>           name = p + 1;
>           p = strchr(name, '/');
>           namelen = p != NULL ? p - name : strlen(name);
> @@ -584,7 +584,7 @@ int qemu_fdt_add_path(void *fdt, const char *path)
>           }
>   
>           parent = retval;
> -    }
> +    } while (p);
>   
>       return retval;
>   }
> 

I think to add a "g_assert_nonull(p);" before the loop would inform the compiler that we always go 
in the loop and remove the compiler error.

Thanks,
Laurent