tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

[PATCH] tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

Posted by Eder Zulian 1 month, 2 weeks ago

- tools/bpf/resolve_btfids/main.c: Initialize 'set' and 'set8' pointers
  to NULL in to fix compiler warnings.

- tools/lib/bpf/btf_dump.c: Initialize 'new_off' and 'pad_bits' to 0 and
  'pad_type' to  NULL to prevent compiler warnings.

- tools/lib/subcmd/parse-options.c: Initiazlide pointer 'o' to NULL
  avoiding a compiler warning.

Tested on x86_64 with clang version 17.0.6 and gcc (GCC) 13.3.1.

$ cd tools/bpf/resolve_btfids
$ for c in gcc clang; do \
for o in fast g s z $(seq 0 3); do \
make clean && \
make HOST_CC=${c} "HOSTCFLAGS=-O${o} -Wall" 2>&1 | tee ${c}-O${o}.out; \
done; done && grep 'warning:\|error:' *.out

[...]
clang-O1.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
clang-O1.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
clang-O2.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
clang-O2.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
clang-O3.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
clang-O3.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
clang-Ofast.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
clang-Ofast.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
clang-Og.out:btf_dump.c:903:42: error: ‘new_off’ may be used uninitialized [-Werror=maybe-uninitialized]
clang-Og.out:btf_dump.c:917:25: error: ‘pad_type’ may be used uninitialized [-Werror=maybe-uninitialized]
clang-Og.out:btf_dump.c:930:20: error: ‘pad_bits’ may be used uninitialized [-Werror=maybe-uninitialized]
clang-Os.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
clang-Oz.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
gcc-O1.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-O1.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-O2.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-O2.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-O3.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-O3.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-Ofast.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-Ofast.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
gcc-Og.out:btf_dump.c:903:42: error: ‘new_off’ may be used uninitialized [-Werror=maybe-uninitialized]
gcc-Og.out:btf_dump.c:917:25: error: ‘pad_type’ may be used uninitialized [-Werror=maybe-uninitialized]
gcc-Og.out:btf_dump.c:930:20: error: ‘pad_bits’ may be used uninitialized [-Werror=maybe-uninitialized]
gcc-Os.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
gcc-Oz.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]

The above warnings and/or errors are not observed after applying this
patch.

Signed-off-by: Eder Zulian <ezulian@redhat.com>
---
 tools/bpf/resolve_btfids/main.c  | 4 ++--
 tools/lib/bpf/btf_dump.c         | 4 ++--
 tools/lib/subcmd/parse-options.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index d54aaa0619df..bd9f960bce3d 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -679,8 +679,8 @@ static int sets_patch(struct object *obj)
 
 	next = rb_first(&obj->sets);
 	while (next) {
-		struct btf_id_set8 *set8;
-		struct btf_id_set *set;
+		struct btf_id_set8 *set8 = NULL;
+		struct btf_id_set *set = NULL;
 		unsigned long addr, off;
 		struct btf_id *id;
 
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 8440c2c5ad3e..468392f9882d 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -867,8 +867,8 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
 	} pads[] = {
 		{"long", d->ptr_sz * 8}, {"int", 32}, {"short", 16}, {"char", 8}
 	};
-	int new_off, pad_bits, bits, i;
-	const char *pad_type;
+	int new_off = 0, pad_bits = 0, bits, i;
+	const char *pad_type = NULL;
 
 	if (cur_off >= next_off)
 		return; /* no gap */
diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
index eb896d30545b..555d617c1f50 100644
--- a/tools/lib/subcmd/parse-options.c
+++ b/tools/lib/subcmd/parse-options.c
@@ -807,7 +807,7 @@ static int option__cmp(const void *va, const void *vb)
 static struct option *options__order(const struct option *opts)
 {
 	int nr_opts = 0, nr_group = 0, nr_parent = 0, len;
-	const struct option *o, *p = opts;
+	const struct option *o = NULL, *p = opts;
 	struct option *opt, *ordered = NULL, *group;
 
 	/* flatten the options that have parents */
-- 
2.46.2

Re: [PATCH] tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

Posted by Yonghong Song 1 month, 1 week ago

On 10/11/24 1:00 PM, Eder Zulian wrote:
> - tools/bpf/resolve_btfids/main.c: Initialize 'set' and 'set8' pointers
>    to NULL in to fix compiler warnings.
>
> - tools/lib/bpf/btf_dump.c: Initialize 'new_off' and 'pad_bits' to 0 and
>    'pad_type' to  NULL to prevent compiler warnings.
>
> - tools/lib/subcmd/parse-options.c: Initiazlide pointer 'o' to NULL
>    avoiding a compiler warning.
>
> Tested on x86_64 with clang version 17.0.6 and gcc (GCC) 13.3.1.
>
> $ cd tools/bpf/resolve_btfids
> $ for c in gcc clang; do \
> for o in fast g s z $(seq 0 3); do \
> make clean && \
> make HOST_CC=${c} "HOSTCFLAGS=-O${o} -Wall" 2>&1 | tee ${c}-O${o}.out; \
> done; done && grep 'warning:\|error:' *.out
>
> [...]
> clang-O1.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O1.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O2.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O2.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O3.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O3.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-Ofast.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-Ofast.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-Og.out:btf_dump.c:903:42: error: ‘new_off’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Og.out:btf_dump.c:917:25: error: ‘pad_type’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Og.out:btf_dump.c:930:20: error: ‘pad_bits’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Os.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Oz.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-O1.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O1.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O2.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O2.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O3.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O3.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-Ofast.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-Ofast.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-Og.out:btf_dump.c:903:42: error: ‘new_off’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Og.out:btf_dump.c:917:25: error: ‘pad_type’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Og.out:btf_dump.c:930:20: error: ‘pad_bits’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Os.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Oz.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
>
> The above warnings and/or errors are not observed after applying this
> patch.
>
> Signed-off-by: Eder Zulian <ezulian@redhat.com>

Currently compilation is okay although it uses -O0.

One choice is to move from -O0 to -O2 and fix corresponding -Wmaybe-uninitialized issues.
Otherwise, since there is no bug, not sure whether we should make changes or not.
Maintainers can decide the next step.

Re: [PATCH] tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

Posted by Viktor Malik 1 month, 1 week ago

On 10/11/24 22:00, Eder Zulian wrote:
> - tools/bpf/resolve_btfids/main.c: Initialize 'set' and 'set8' pointers
>   to NULL in to fix compiler warnings.
> 
> - tools/lib/bpf/btf_dump.c: Initialize 'new_off' and 'pad_bits' to 0 and
>   'pad_type' to  NULL to prevent compiler warnings.
> 
> - tools/lib/subcmd/parse-options.c: Initiazlide pointer 'o' to NULL

Typo: "Initiazlide" -> "Initialize"

>   avoiding a compiler warning.

I think that the three changes should be split into three patches so
that we don't have one patch touching three different tools.

Then, maintainers can also decide whether they prefer your patch of
subcmd or the one mentioned in the other thread.

Viktor

> 
> Tested on x86_64 with clang version 17.0.6 and gcc (GCC) 13.3.1.
> 
> $ cd tools/bpf/resolve_btfids
> $ for c in gcc clang; do \
> for o in fast g s z $(seq 0 3); do \
> make clean && \
> make HOST_CC=${c} "HOSTCFLAGS=-O${o} -Wall" 2>&1 | tee ${c}-O${o}.out; \
> done; done && grep 'warning:\|error:' *.out
> 
> [...]
> clang-O1.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O1.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O2.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O2.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O3.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-O3.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-Ofast.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-Ofast.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> clang-Og.out:btf_dump.c:903:42: error: ‘new_off’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Og.out:btf_dump.c:917:25: error: ‘pad_type’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Og.out:btf_dump.c:930:20: error: ‘pad_bits’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Os.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> clang-Oz.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-O1.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O1.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O2.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O2.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O3.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-O3.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-Ofast.out:main.c:163:9: warning: ‘set8’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-Ofast.out:main.c:163:9: warning: ‘set’ may be used uninitialized [-Wmaybe-uninitialized]
> gcc-Og.out:btf_dump.c:903:42: error: ‘new_off’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Og.out:btf_dump.c:917:25: error: ‘pad_type’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Og.out:btf_dump.c:930:20: error: ‘pad_bits’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Os.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> gcc-Oz.out:parse-options.c:832:9: error: ‘o’ may be used uninitialized [-Werror=maybe-uninitialized]
> 
> The above warnings and/or errors are not observed after applying this
> patch.
> 
> Signed-off-by: Eder Zulian <ezulian@redhat.com>
> ---
>  tools/bpf/resolve_btfids/main.c  | 4 ++--
>  tools/lib/bpf/btf_dump.c         | 4 ++--
>  tools/lib/subcmd/parse-options.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index d54aaa0619df..bd9f960bce3d 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -679,8 +679,8 @@ static int sets_patch(struct object *obj)
>  
>  	next = rb_first(&obj->sets);
>  	while (next) {
> -		struct btf_id_set8 *set8;
> -		struct btf_id_set *set;
> +		struct btf_id_set8 *set8 = NULL;
> +		struct btf_id_set *set = NULL;
>  		unsigned long addr, off;
>  		struct btf_id *id;
>  
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 8440c2c5ad3e..468392f9882d 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -867,8 +867,8 @@ static void btf_dump_emit_bit_padding(const struct btf_dump *d,
>  	} pads[] = {
>  		{"long", d->ptr_sz * 8}, {"int", 32}, {"short", 16}, {"char", 8}
>  	};
> -	int new_off, pad_bits, bits, i;
> -	const char *pad_type;
> +	int new_off = 0, pad_bits = 0, bits, i;
> +	const char *pad_type = NULL;
>  
>  	if (cur_off >= next_off)
>  		return; /* no gap */
> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
> index eb896d30545b..555d617c1f50 100644
> --- a/tools/lib/subcmd/parse-options.c
> +++ b/tools/lib/subcmd/parse-options.c
> @@ -807,7 +807,7 @@ static int option__cmp(const void *va, const void *vb)
>  static struct option *options__order(const struct option *opts)
>  {
>  	int nr_opts = 0, nr_group = 0, nr_parent = 0, len;
> -	const struct option *o, *p = opts;
> +	const struct option *o = NULL, *p = opts;
>  	struct option *opt, *ordered = NULL, *group;
>  
>  	/* flatten the options that have parents */

Re: [PATCH] tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

Posted by Sam James 1 month, 2 weeks ago

The parse-options change was sent before as
https://lore.kernel.org/all/20240731085217.94928-1-michael.weiss@aisec.fraunhofer.de/
but seems to have fallen through the cracks.

Re: [PATCH] tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

Posted by Eder Zulian 1 month, 2 weeks ago

Hi Sam, thank you for pointing it out.

On Sat, Oct 12, 2024 at 05:14:29AM +0100, Sam James wrote:
> The parse-options change was sent before as
> https://lore.kernel.org/all/20240731085217.94928-1-michael.weiss@aisec.fraunhofer.de/

Sorry, I missed Michael's patch.
My suggestion is to initialize 'o' to NULL instead. An illegal dereferencing
(if any) would then be evident.

> but seems to have fallen through the cracks.
> 
> 
Would it be better to revert this part and wait a bit for Michael's patch to
be merged, please let me know.

Thank you,
Eder

Re: [PATCH] tools/resolve_btfids: Fix 'variable' may be used uninitialized warnings

Posted by Sam James 1 month, 2 weeks ago

Eder Zulian <ezulian@redhat.com> writes:

> Hi Sam, thank you for pointing it out.
>
> On Sat, Oct 12, 2024 at 05:14:29AM +0100, Sam James wrote:
>> The parse-options change was sent before as
>> https://lore.kernel.org/all/20240731085217.94928-1-michael.weiss@aisec.fraunhofer.de/
>
> Sorry, I missed Michael's patch.
> My suggestion is to initialize 'o' to NULL instead. An illegal dereferencing
> (if any) would then be evident.

Yeah, I was wondering the same.

>
>> but seems to have fallen through the cracks.
>> 
>> 
> Would it be better to revert this part and wait a bit for Michael's patch to
> be merged, please let me know.

I'll defer to the maintainers.

>
> Thank you,
> Eder