This is a follow up patch for commit 495d2d8133fd("selftests/bpf: Attempt
to build BPF programs with -Wsign-compare") from Alexei Starovoitov[1]
to be able to enable -Wsign-compare C compilation flag for clang since
-Wall doesn't add it and BPF programs are built with clang.This has the
benefit to catch problematic comparisons in future tests as quoted from
the commit message:"
int i = -1;
unsigned int j = 1;
if (i < j) // this is false.
long i = -1;
unsigned int j = 1;
if (i < j) // this is true.
C standard for reference:
- If either operand is unsigned long the other shall be converted to
unsigned long.
- Otherwise, if one operand is a long int and the other unsigned int,
then if a long int can represent all the values of an unsigned int,
the unsigned int shall be converted to a long int;
otherwise both operands shall be converted to unsigned long int.
- Otherwise, if either operand is long, the other shall be
converted to long.
- Otherwise, if either operand is unsigned, the other shall be
converted to unsigned.
Unfortunately clang's -Wsign-compare is very noisy.
It complains about (s32)a == (u32)b which is safe and doen't
have surprising behavior."
This specific patch supresses the following warnings when
-Wsign-compare is enabled:
1 warning generated.
progs/bpf_iter_bpf_percpu_array_map.c:35:16: warning: comparison of
integers of different signs: 'int' and 'const volatile __u32'
(aka 'const volatile unsigned int') [-Wsign-compare]
35 | for (i = 0; i < num_cpus; i++) {
| ~ ^ ~~~~~~~~
1 warning generated.
progs/bpf_qdisc_fifo.c:93:2: warning: comparison of integers of
different signs: 'int' and '__u32'
(aka 'unsigned int') [-Wsign-compare]
93 | bpf_for(i, 0, sch->q.qlen) {
| ^ ~ ~~~~~~~~~~~
Should be noted that many more similar changes are still needed in order
to be able to enable the -Wsign-compare flag since -Werror is enabled and
would cause compilation of bpf selftests to fail.
[1].
Link:https://github.com/torvalds/linux/commit/495d2d8133fd1407519170a5238f455abbd9ec9b
Signed-off-by: Mehdi Ben Hadj Khelifa <[email protected]>
---
Changelog:
Changes from v3:
-Downsized the patch as suggested by vivek yadav[2].
-Changed the commit message as suggested by Daniel Borkmann[3].
Link:https://lore.kernel.org/all/[email protected]/#r
Changes from v2:
-Split up the patch into a patch series as suggested by vivek
-Include only changes to variable types with no casting by my mentor
david
-Removed the -Wsign-compare in Makefile to avoid compilation errors
until adding casting for rest of comparisons.
Link:https://lore.kernel.org/bpf/[email protected]/T/#u
Changes from v1:
- Fix CI failed builds where it failed due to do missing .c and
.h files in my patch for working in mainline.
Link:https://lore.kernel.org/bpf/[email protected]/T/#u
[2]:https://lore.kernel.org/all/CABPSWR7_w3mxr74wCDEF=myyug2f_vmjed-dqotc8mdmas_...@mail.gmail.com/
[3]:https://lore.kernel.org/all/[email protected]/
.../testing/selftests/bpf/progs/bpf_iter_bpf_percpu_array_map.c | 2 +-
tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_array_map.c
b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_array_map.c
index 9fdea8cd4c6f..0baf00463f35 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_array_map.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_percpu_array_map.c
@@ -24,7 +24,7 @@ int dump_bpf_percpu_array_map(struct bpf_iter__bpf_map_elem
*ctx)
__u32 *key = ctx->key;
void *pptr = ctx->value;
__u32 step;
- int i;
+ __u32 i;
if (key == (void *)0 || pptr == (void *)0)
return 0;
diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
b/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
index 1de2be3e370b..7a639dcb23a9 100644
--- a/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
+++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
@@ -88,7 +88,7 @@ void BPF_PROG(bpf_fifo_reset, struct Qdisc *sch)
{
struct bpf_list_node *node;
struct skb_node *skbn;
- int i;
+ __u32 i;
bpf_for(i, 0, sch->q.qlen) {
struct sk_buff *skb = NULL;
--
2.51.1.dirty
