| Message ID | 20211108200756.1302697-1-sw@weilnetz.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | device_tree: Fix compiler error | expand |
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 > >
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
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~
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
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
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; }
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(-)