[PATCH v1] tools build: Fix a number of Wconversion warnings

[PATCH v1] tools build: Fix a number of Wconversion warnings

[Ian Rogers]

There's some expressed interest in having the compiler flag
-Wconversion detect at build time certain kinds of potential problems:
https://lore.kernel.org/lkml/20250103182532.GB781381@e132581.arm.com/

As feature detection passes -Wconversion from CFLAGS when set, the
feature detection compile tests need to not fail because of
-Wconversion as the failure will be interpretted as a missing
feature. Switch various types to avoid the -Wconversion issue, the
exact meaning of the code is unimportant as it is typically looking
for header file definitions.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/build/feature/test-backtrace.c           | 2 +-
 tools/build/feature/test-bpf.c                 | 2 +-
 tools/build/feature/test-glibc.c               | 2 +-
 tools/build/feature/test-libdebuginfod.c       | 2 +-
 tools/build/feature/test-libdw.c               | 2 +-
 tools/build/feature/test-libelf-gelf_getnote.c | 2 +-
 tools/build/feature/test-libelf.c              | 2 +-
 tools/build/feature/test-lzma.c                | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/build/feature/test-backtrace.c b/tools/build/feature/test-backtrace.c
index e9ddd27c69c3..7962fbad6401 100644
--- a/tools/build/feature/test-backtrace.c
+++ b/tools/build/feature/test-backtrace.c
@@ -5,7 +5,7 @@
 int main(void)
 {
 	void *backtrace_fns[10];
-	size_t entries;
+	int entries;
 
 	entries = backtrace(backtrace_fns, 10);
 	backtrace_symbols_fd(backtrace_fns, entries, 1);
diff --git a/tools/build/feature/test-bpf.c b/tools/build/feature/test-bpf.c
index 727d22e34a6e..e7a405f83af6 100644
--- a/tools/build/feature/test-bpf.c
+++ b/tools/build/feature/test-bpf.c
@@ -44,5 +44,5 @@ int main(void)
 	 * Test existence of __NR_bpf and BPF_PROG_LOAD.
 	 * This call should fail if we run the testcase.
 	 */
-	return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
+	return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr)) == 0;
 }
diff --git a/tools/build/feature/test-glibc.c b/tools/build/feature/test-glibc.c
index 9ab8e90e7b88..20a250419f31 100644
--- a/tools/build/feature/test-glibc.c
+++ b/tools/build/feature/test-glibc.c
@@ -16,5 +16,5 @@ int main(void)
 	const char *version = XSTR(__GLIBC__) "." XSTR(__GLIBC_MINOR__);
 #endif
 
-	return (long)version;
+	return version == NULL;
 }
diff --git a/tools/build/feature/test-libdebuginfod.c b/tools/build/feature/test-libdebuginfod.c
index da22548b8413..823f9fa9391d 100644
--- a/tools/build/feature/test-libdebuginfod.c
+++ b/tools/build/feature/test-libdebuginfod.c
@@ -4,5 +4,5 @@
 int main(void)
 {
 	debuginfod_client* c = debuginfod_begin();
-	return (long)c;
+	return !!c;
 }
diff --git a/tools/build/feature/test-libdw.c b/tools/build/feature/test-libdw.c
index 2fb59479ab77..aabd63ca76b4 100644
--- a/tools/build/feature/test-libdw.c
+++ b/tools/build/feature/test-libdw.c
@@ -9,7 +9,7 @@ int test_libdw(void)
 {
 	Dwarf *dbg = dwarf_begin(0, DWARF_C_READ);
 
-	return (long)dbg;
+	return dbg == NULL;
 }
 
 int test_libdw_unwind(void)
diff --git a/tools/build/feature/test-libelf-gelf_getnote.c b/tools/build/feature/test-libelf-gelf_getnote.c
index 075d062fe841..e06121161161 100644
--- a/tools/build/feature/test-libelf-gelf_getnote.c
+++ b/tools/build/feature/test-libelf-gelf_getnote.c
@@ -4,5 +4,5 @@
 
 int main(void)
 {
-	return gelf_getnote(NULL, 0, NULL, NULL, NULL);
+	return gelf_getnote(NULL, 0, NULL, NULL, NULL) == 0;
 }
diff --git a/tools/build/feature/test-libelf.c b/tools/build/feature/test-libelf.c
index 905044127d56..2dbb6ea870f3 100644
--- a/tools/build/feature/test-libelf.c
+++ b/tools/build/feature/test-libelf.c
@@ -5,5 +5,5 @@ int main(void)
 {
 	Elf *elf = elf_begin(0, ELF_C_READ, 0);
 
-	return (long)elf;
+	return !!elf;
 }
diff --git a/tools/build/feature/test-lzma.c b/tools/build/feature/test-lzma.c
index 78682bb01d57..b57103774e8e 100644
--- a/tools/build/feature/test-lzma.c
+++ b/tools/build/feature/test-lzma.c
@@ -4,7 +4,7 @@
 int main(void)
 {
 	lzma_stream strm = LZMA_STREAM_INIT;
-	int ret;
+	lzma_ret ret;
 
 	ret = lzma_stream_decoder(&strm, UINT64_MAX, LZMA_CONCATENATED);
 	return ret ? -1 : 0;
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v1] tools build: Fix a number of Wconversion warnings

[James Clark]

On 06/01/2025 9:54 pm, Ian Rogers wrote:
> There's some expressed interest in having the compiler flag
> -Wconversion detect at build time certain kinds of potential problems:
> https://lore.kernel.org/lkml/20250103182532.GB781381@e132581.arm.com/
> 
> As feature detection passes -Wconversion from CFLAGS when set, the
> feature detection compile tests need to not fail because of
> -Wconversion as the failure will be interpretted as a missing
> feature. Switch various types to avoid the -Wconversion issue, the
> exact meaning of the code is unimportant as it is typically looking
> for header file definitions.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

What's the plan for errors in #includes that we can't modify? I noticed 
the Perl feature test fails with -Wconversion but can be fixed by 
disabling the warning:

   #pragma GCC diagnostic push
   #pragma GCC diagnostic ignored "-Wsign-conversion"
   #pragma GCC diagnostic ignored "-Wconversion"
   #include <EXTERN.h>
   #include <perl.h>
   #pragma GCC diagnostic pop

Not sure why it needs both those things to be disabled when I only 
enabled -Wconversion, but it does.

> ---
>   tools/build/feature/test-backtrace.c           | 2 +-
>   tools/build/feature/test-bpf.c                 | 2 +-
>   tools/build/feature/test-glibc.c               | 2 +-
>   tools/build/feature/test-libdebuginfod.c       | 2 +-
>   tools/build/feature/test-libdw.c               | 2 +-
>   tools/build/feature/test-libelf-gelf_getnote.c | 2 +-
>   tools/build/feature/test-libelf.c              | 2 +-
>   tools/build/feature/test-lzma.c                | 2 +-
>   8 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/build/feature/test-backtrace.c b/tools/build/feature/test-backtrace.c
> index e9ddd27c69c3..7962fbad6401 100644
> --- a/tools/build/feature/test-backtrace.c
> +++ b/tools/build/feature/test-backtrace.c
> @@ -5,7 +5,7 @@
>   int main(void)
>   {
>   	void *backtrace_fns[10];
> -	size_t entries;
> +	int entries;
>   
>   	entries = backtrace(backtrace_fns, 10);
>   	backtrace_symbols_fd(backtrace_fns, entries, 1);
> diff --git a/tools/build/feature/test-bpf.c b/tools/build/feature/test-bpf.c
> index 727d22e34a6e..e7a405f83af6 100644
> --- a/tools/build/feature/test-bpf.c
> +++ b/tools/build/feature/test-bpf.c
> @@ -44,5 +44,5 @@ int main(void)
>   	 * Test existence of __NR_bpf and BPF_PROG_LOAD.
>   	 * This call should fail if we run the testcase.
>   	 */
> -	return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
> +	return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr)) == 0;

Seems a bit weird to invert some of the return values rather than doing 
!= 0, but as you say, the actual values seem to be unimportant.

Reviewed-by: James Clark <james.clark@linaro.org>

>   }
> diff --git a/tools/build/feature/test-glibc.c b/tools/build/feature/test-glibc.c
> index 9ab8e90e7b88..20a250419f31 100644
> --- a/tools/build/feature/test-glibc.c
> +++ b/tools/build/feature/test-glibc.c
> @@ -16,5 +16,5 @@ int main(void)
>   	const char *version = XSTR(__GLIBC__) "." XSTR(__GLIBC_MINOR__);
>   #endif
>   
> -	return (long)version;
> +	return version == NULL;
>   }
> diff --git a/tools/build/feature/test-libdebuginfod.c b/tools/build/feature/test-libdebuginfod.c
> index da22548b8413..823f9fa9391d 100644
> --- a/tools/build/feature/test-libdebuginfod.c
> +++ b/tools/build/feature/test-libdebuginfod.c
> @@ -4,5 +4,5 @@
>   int main(void)
>   {
>   	debuginfod_client* c = debuginfod_begin();
> -	return (long)c;
> +	return !!c;
>   }
> diff --git a/tools/build/feature/test-libdw.c b/tools/build/feature/test-libdw.c
> index 2fb59479ab77..aabd63ca76b4 100644
> --- a/tools/build/feature/test-libdw.c
> +++ b/tools/build/feature/test-libdw.c
> @@ -9,7 +9,7 @@ int test_libdw(void)
>   {
>   	Dwarf *dbg = dwarf_begin(0, DWARF_C_READ);
>   
> -	return (long)dbg;
> +	return dbg == NULL;
>   }
>   
>   int test_libdw_unwind(void)
> diff --git a/tools/build/feature/test-libelf-gelf_getnote.c b/tools/build/feature/test-libelf-gelf_getnote.c
> index 075d062fe841..e06121161161 100644
> --- a/tools/build/feature/test-libelf-gelf_getnote.c
> +++ b/tools/build/feature/test-libelf-gelf_getnote.c
> @@ -4,5 +4,5 @@
>   
>   int main(void)
>   {
> -	return gelf_getnote(NULL, 0, NULL, NULL, NULL);
> +	return gelf_getnote(NULL, 0, NULL, NULL, NULL) == 0;
>   }
> diff --git a/tools/build/feature/test-libelf.c b/tools/build/feature/test-libelf.c
> index 905044127d56..2dbb6ea870f3 100644
> --- a/tools/build/feature/test-libelf.c
> +++ b/tools/build/feature/test-libelf.c
> @@ -5,5 +5,5 @@ int main(void)
>   {
>   	Elf *elf = elf_begin(0, ELF_C_READ, 0);
>   
> -	return (long)elf;
> +	return !!elf;
>   }
> diff --git a/tools/build/feature/test-lzma.c b/tools/build/feature/test-lzma.c
> index 78682bb01d57..b57103774e8e 100644
> --- a/tools/build/feature/test-lzma.c
> +++ b/tools/build/feature/test-lzma.c
> @@ -4,7 +4,7 @@
>   int main(void)
>   {
>   	lzma_stream strm = LZMA_STREAM_INIT;
> -	int ret;
> +	lzma_ret ret;
>   
>   	ret = lzma_stream_decoder(&strm, UINT64_MAX, LZMA_CONCATENATED);
>   	return ret ? -1 : 0;

Re: [PATCH v1] tools build: Fix a number of Wconversion warnings

[Ian Rogers]

On Tue, Jan 7, 2025 at 2:33 AM James Clark <james.clark@linaro.org> wrote:
>
> On 06/01/2025 9:54 pm, Ian Rogers wrote:
> > There's some expressed interest in having the compiler flag
> > -Wconversion detect at build time certain kinds of potential problems:
> > https://lore.kernel.org/lkml/20250103182532.GB781381@e132581.arm.com/
> >
> > As feature detection passes -Wconversion from CFLAGS when set, the
> > feature detection compile tests need to not fail because of
> > -Wconversion as the failure will be interpretted as a missing
> > feature. Switch various types to avoid the -Wconversion issue, the
> > exact meaning of the code is unimportant as it is typically looking
> > for header file definitions.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> What's the plan for errors in #includes that we can't modify? I noticed
> the Perl feature test fails with -Wconversion but can be fixed by
> disabling the warning:
>
>    #pragma GCC diagnostic push
>    #pragma GCC diagnostic ignored "-Wsign-conversion"
>    #pragma GCC diagnostic ignored "-Wconversion"
>    #include <EXTERN.h>
>    #include <perl.h>
>    #pragma GCC diagnostic pop
>
> Not sure why it needs both those things to be disabled when I only
> enabled -Wconversion, but it does.

This change lgtm, I'm not sure how others feel. I don't have a plan, I
was just following up on Leo's Wconversion comment to see what state
things were in. The feature tests without these changes pretty much
break the build (I can live without perl support :-) ) so I thought I
could move things forward there and then see the state of Wconversion
with the patch I was working on.

I'm not sure how others feel about fixing Wconversion in perf, the
errors are quite noisy imo. The biggest issue imo will be with headers
shared by tools and the kernel, where kernel people may be vocal on
the merits of Wconversion.

> > ---
> >   tools/build/feature/test-backtrace.c           | 2 +-
> >   tools/build/feature/test-bpf.c                 | 2 +-
> >   tools/build/feature/test-glibc.c               | 2 +-
> >   tools/build/feature/test-libdebuginfod.c       | 2 +-
> >   tools/build/feature/test-libdw.c               | 2 +-
> >   tools/build/feature/test-libelf-gelf_getnote.c | 2 +-
> >   tools/build/feature/test-libelf.c              | 2 +-
> >   tools/build/feature/test-lzma.c                | 2 +-
> >   8 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/build/feature/test-backtrace.c b/tools/build/feature/test-backtrace.c
> > index e9ddd27c69c3..7962fbad6401 100644
> > --- a/tools/build/feature/test-backtrace.c
> > +++ b/tools/build/feature/test-backtrace.c
> > @@ -5,7 +5,7 @@
> >   int main(void)
> >   {
> >       void *backtrace_fns[10];
> > -     size_t entries;
> > +     int entries;
> >
> >       entries = backtrace(backtrace_fns, 10);
> >       backtrace_symbols_fd(backtrace_fns, entries, 1);
> > diff --git a/tools/build/feature/test-bpf.c b/tools/build/feature/test-bpf.c
> > index 727d22e34a6e..e7a405f83af6 100644
> > --- a/tools/build/feature/test-bpf.c
> > +++ b/tools/build/feature/test-bpf.c
> > @@ -44,5 +44,5 @@ int main(void)
> >        * Test existence of __NR_bpf and BPF_PROG_LOAD.
> >        * This call should fail if we run the testcase.
> >        */
> > -     return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
> > +     return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr)) == 0;
>
> Seems a bit weird to invert some of the return values rather than doing
> != 0, but as you say, the actual values seem to be unimportant.
>
> Reviewed-by: James Clark <james.clark@linaro.org>

Yeah it was arbitrary and I didn't want to add a stdlib.h dependency
in a bunch of places for the sake of a definition of NULL. I'm happy
for things to be done differently if people like.

Thanks,
Ian

Re: [PATCH v1] tools build: Fix a number of Wconversion warnings

[James Clark]

On 07/01/2025 4:11 pm, Ian Rogers wrote:
> On Tue, Jan 7, 2025 at 2:33 AM James Clark <james.clark@linaro.org> wrote:
>>
>> On 06/01/2025 9:54 pm, Ian Rogers wrote:
>>> There's some expressed interest in having the compiler flag
>>> -Wconversion detect at build time certain kinds of potential problems:
>>> https://lore.kernel.org/lkml/20250103182532.GB781381@e132581.arm.com/
>>>
>>> As feature detection passes -Wconversion from CFLAGS when set, the
>>> feature detection compile tests need to not fail because of
>>> -Wconversion as the failure will be interpretted as a missing
>>> feature. Switch various types to avoid the -Wconversion issue, the
>>> exact meaning of the code is unimportant as it is typically looking
>>> for header file definitions.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>
>> What's the plan for errors in #includes that we can't modify? I noticed
>> the Perl feature test fails with -Wconversion but can be fixed by
>> disabling the warning:
>>
>>     #pragma GCC diagnostic push
>>     #pragma GCC diagnostic ignored "-Wsign-conversion"
>>     #pragma GCC diagnostic ignored "-Wconversion"
>>     #include <EXTERN.h>
>>     #include <perl.h>
>>     #pragma GCC diagnostic pop
>>
>> Not sure why it needs both those things to be disabled when I only
>> enabled -Wconversion, but it does.
> 
> This change lgtm, I'm not sure how others feel. I don't have a plan, I
> was just following up on Leo's Wconversion comment to see what state
> things were in. The feature tests without these changes pretty much
> break the build (I can live without perl support :-) ) so I thought I
> could move things forward there and then see the state of Wconversion
> with the patch I was working on.
> 
> I'm not sure how others feel about fixing Wconversion in perf, the
> errors are quite noisy imo. The biggest issue imo will be with headers
> shared by tools and the kernel, where kernel people may be vocal on
> the merits of Wconversion.
> 

More warnings are better IMO, but I can't say I have any experience with 
that particular one, whether it's too painful to be useful or not.

And if it's going to require those pragmas all over the place to fully 
enable it maybe it's better to leave it off. Just your change without 
the pragmas makes sense for sporadic manual testing though, and we can 
leave libperl disabled.

>>> ---
>>>    tools/build/feature/test-backtrace.c           | 2 +-
>>>    tools/build/feature/test-bpf.c                 | 2 +-
>>>    tools/build/feature/test-glibc.c               | 2 +-
>>>    tools/build/feature/test-libdebuginfod.c       | 2 +-
>>>    tools/build/feature/test-libdw.c               | 2 +-
>>>    tools/build/feature/test-libelf-gelf_getnote.c | 2 +-
>>>    tools/build/feature/test-libelf.c              | 2 +-
>>>    tools/build/feature/test-lzma.c                | 2 +-
>>>    8 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tools/build/feature/test-backtrace.c b/tools/build/feature/test-backtrace.c
>>> index e9ddd27c69c3..7962fbad6401 100644
>>> --- a/tools/build/feature/test-backtrace.c
>>> +++ b/tools/build/feature/test-backtrace.c
>>> @@ -5,7 +5,7 @@
>>>    int main(void)
>>>    {
>>>        void *backtrace_fns[10];
>>> -     size_t entries;
>>> +     int entries;
>>>
>>>        entries = backtrace(backtrace_fns, 10);
>>>        backtrace_symbols_fd(backtrace_fns, entries, 1);
>>> diff --git a/tools/build/feature/test-bpf.c b/tools/build/feature/test-bpf.c
>>> index 727d22e34a6e..e7a405f83af6 100644
>>> --- a/tools/build/feature/test-bpf.c
>>> +++ b/tools/build/feature/test-bpf.c
>>> @@ -44,5 +44,5 @@ int main(void)
>>>         * Test existence of __NR_bpf and BPF_PROG_LOAD.
>>>         * This call should fail if we run the testcase.
>>>         */
>>> -     return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
>>> +     return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr)) == 0;
>>
>> Seems a bit weird to invert some of the return values rather than doing
>> != 0, but as you say, the actual values seem to be unimportant.
>>
>> Reviewed-by: James Clark <james.clark@linaro.org>
> 
> Yeah it was arbitrary and I didn't want to add a stdlib.h dependency
> in a bunch of places for the sake of a definition of NULL. I'm happy
> for things to be done differently if people like.
> 
> Thanks,
> Ian

Re: [PATCH v1] tools build: Fix a number of Wconversion warnings

[Ian Rogers]

On Mon, Jan 6, 2025 at 1:54 PM Ian Rogers <irogers@google.com> wrote:
>
> There's some expressed interest in having the compiler flag
> -Wconversion detect at build time certain kinds of potential problems:
> https://lore.kernel.org/lkml/20250103182532.GB781381@e132581.arm.com/
>
> As feature detection passes -Wconversion from CFLAGS when set, the
> feature detection compile tests need to not fail because of
> -Wconversion as the failure will be interpretted as a missing
> feature. Switch various types to avoid the -Wconversion issue, the
> exact meaning of the code is unimportant as it is typically looking
> for header file definitions.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Ping. Also:
Reviewed-by: James Clark <james.clark@linaro.org>
Would be nice to be cleaner wrt compiler warnings.

Thanks,
Ian

Re: [PATCH v1] tools build: Fix a number of Wconversion warnings

[Namhyung Kim]

On Mon, 06 Jan 2025 13:54:42 -0800, Ian Rogers wrote:
> There's some expressed interest in having the compiler flag
> -Wconversion detect at build time certain kinds of potential problems:
> https://lore.kernel.org/lkml/20250103182532.GB781381@e132581.arm.com/
> 
> As feature detection passes -Wconversion from CFLAGS when set, the
> feature detection compile tests need to not fail because of
> -Wconversion as the failure will be interpretted as a missing
> feature. Switch various types to avoid the -Wconversion issue, the
> exact meaning of the code is unimportant as it is typically looking
> for header file definitions.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung