On Mon, 29 Sep 2025 17:03:29 +0100
Mehdi Ben Hadj Khelifa [off-list ref] wrote:

On 9/26/25 12:45 PM, David Laight wrote:

quoted

On Wed, 24 Sep 2025 17:23:49 +0100
Mehdi Ben Hadj Khelifa [off-list ref] wrote:
  

quoted

-Change all the source files and the corresponding headers
to having matching sign comparisons.  

Hi david,
sorry for the late reply.

quoted

'Fixing' -Wsign-compare by adding loads of casts doesn't seem right.
The only real way is to change all the types to unsigned ones.  

The last v3 did only do that with no casting as it was suggested by 
David too.

quoted

Consider the following:
	int x = read(fd, buf, len);
	if (x < 0)
		return -1;
	if (x > sizeof (struct fubar))
		return -1;
That will generate a 'sign-compare' error, but min(x, sizeof (struct fubar))
doesn't generate an error because the compiler knows 'x' isn't negative.  

  Yes,-Wsign-compare does add errors with -Werror enabled in that case 
and many other cases where the code is perfectly fine which is one of 
it's drawbacks.
Also I though that because of GCC/Clang heuristics 
sometimes min() suppress the warning not because that the compiler knows 
that x isn't negative.I'm probably wrong here.

That sentence doesn't make sense.
The statically_true() test in min() uses the 'value' tracking done by modern
versions of gcc and clang.
This means it can let signed types be promoted to unsigned ones because the
compiler knows the value isn't negative.
OTOH -Wsign-compare is a much older warning and is only based on the types.

quoted

A well known compiler also rejects:
	unsigned char a;
	unsigned int b;
	if (b > a)
		return;
because 'a' is promoted to 'signed int' before it does the check.  

In my knowledge,compilers don't necessarily reject the above code by 
default. Since -Wall in GCC includes -Wsign-compare but -Wall in clang 
doesn't, doing -Wall -Werror for clang compiler won't trigger an error 
in the case above not even a warning.My changes are to make those 
comparisons produce an error since the -Werror flag is already enabled 
in the Makefile.

This isn't about whether -Wsign-compare is enabled or not (or even what
the option is called).
It is about whether the compiler's 'sign-compare' warning triggers for that code.
The one that detects the warning/error isn't gcc or clang but is probably
used far more than clang.

quoted

So until the compilers start looking at the known domain of the value
(not just the type) I enabling -Wsign-compare' is pretty pointless.  

I agree that enabling -Wsign-compare is pretty noisy. But it does have 
some usefulness. Take for example this code:
	int n = -5;
	for (unsigned i = 0; i < n; i++) {
     	// ...
	}
Since this is valid code by the compiler, it will allow it but n here is 
promoted to an unsigned which converts -5 to being 4294967291 thus 
making the loop run more than what was desired.of course,here the 
example is much easy to follow and variables are very well set but the 
point is that these could cause issues when hidden inside a lot of macro 
code.

There is plenty of broken code out there.
It isn't hard to find places where explicit casts make things worse.
The problem is that, even for the above example, the -5 could come from
way earlier up the code.
If you 'fix' the warning by changing it to 'i < (unsigned)n' the code is
still just as likely to be buggy.

quoted

As a matter of interest did you actually find any bugs?  

No,I have not found any bug related to the current state of code in bpf 
selftests but It works as a prevention mechanism for future bugs.Rather 
than wait until something breaks in future code.

That's what I expected...

quoted

	David
  

Thank you for your time David.I would appreciate if you suggest on how I 
can have a useful patch on this or if I should drop this.
Best Regards,
Mehdi

quoted

quoted

Signed-off-by: Mehdi Ben Hadj Khelifa <redacted>
---
As suggested by the TODO, -Wsign-compare was added to the C compilation
flags for the selftests/bpf/Makefile and all corresponding files in
selftests and a single file under tools/lib/bpf/usdt.bpf.h have been
carefully changed to account for correct sign comparisons either by
explicit casting or changing the variable type.Only local variables
and variables which are in limited scope have been changed in cases
where it doesn't break the code.Other struct variables or global ones
have left untouched to avoid other conflicts and opted to explicit
casting in this case.This change will help avoid implicit type
conversions and have predictable behavior.

I have already compiled all bpf tests with no errors as well as the
kernel and have ran all the selftests with no obvious side effects.
I would like to know if it's more convinient to have all changes as
a single patch like here or if it needs to be divided in some way
and sent as a patch series.

Best Regards,
Mehdi Ben Hadj Khelifa  

...