Page MenuHomeFreeBSD

Appease -Wsign-compare in radix.c
ClosedPublic

Authored by zec on Apr 8 2021, 9:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 17, 12:26 AM
Unknown Object (File)
Dec 14 2023, 10:48 PM
Unknown Object (File)
Dec 1 2023, 11:52 AM
Unknown Object (File)
Nov 20 2023, 6:40 AM
Unknown Object (File)
Nov 2 2023, 8:32 PM
Unknown Object (File)
Oct 13 2023, 8:35 AM
Unknown Object (File)
Sep 30 2023, 8:12 PM
Unknown Object (File)
Aug 16 2023, 9:58 AM

Details

Summary

Permit compiling with -W, which comes handy when compiling radix.c in (userland) applications with stricter build policies.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

zec requested review of this revision.Apr 8 2021, 9:40 PM

May you please provide a full context diff?
I can't see, why the compiler issued a warning and how the fix will break other things (i.e. logic errors)

Expand the diff to -U30 per request from donner@.

May you please provide a full context diff?
I can't see, why the compiler issued a warning and how the fix will break other things (i.e. logic errors)

Diff updated, the build breaks at:

../../../net/radix.c:459:13: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

} while (b > (unsigned) x->rn_bit);
         ~ ^ ~~~~~~~~~~~~~~~~~~~~
donner requested changes to this revision.Apr 9 2021, 6:58 AM

The variable b and the struct member rn_bit are both signed values because they are supposed to hold negative numbers.

sys/net/radix.c
459

In this special case, the negative values of rn_bit are considered as absurd high values, while b is used as a signed value later on.

459

The correct way might be something like

b > x->rn_bit && x->rn_bit >= 0
This revision now requires changes to proceed.Apr 9 2021, 6:58 AM
zec added inline comments.
sys/net/radix.c
459

The correct way might be something like

b > x->rn_bit && x->rn_bit >= 0

This is exactly what the original author (@rgrimes? ksklower?) left in a comment a line below. The current form of that condition:

b > (unsigned) x->rn_bit

looks like an optimization which removes one branch. The problem is that the compiler silently converts (signed) b to unsigned, but complaints if -Wsign-compare is turned on. If you're concerned with declaring b as unsigned, we could simply make the current reality more explicit:

(unsigned) b > (unsigned) x->rn_bit

which looks plain ugly to me, but is guaranteed to be a no-op change, and will silence the -Wsign-compare, which is the end goal I want to achieve.

459

In this special case, the negative values of rn_bit are considered as absurd high values, while b is used as a signed value later on.

Without pretending I actually understand everything what the existing code does, I fail to see how b could become negative here, and the fact that it is later used unmodified as an argument to rn_newpair() can't make it negative / overflow either.

sys/net/radix.c
459

looks like an optimization which removes one branch.

Yes it is.

The problem is that the compiler silently converts (signed) b to unsigned, but complaints if -Wsign-compare is turned on. If you're concerned with declaring b as unsigned, we could simply make the current reality more explicit:

(unsigned) b > (unsigned) x->rn_bit

which looks plain ugly to me, but is guaranteed to be a no-op change, and will silence the -Wsign-compare, which is the end goal I want to achieve.

This is not logical equivalent to the original statement. It's more

(b > x->rn_bit && x->rn_bit >= 0 && b >= 0) ||
(b < x->rn_bit && x->rn_bit <  0 && b <  0)
sys/net/radix.c
459

Without pretending I actually understand everything what the existing code does, I fail to see how b could become negative here, and the fact that it is later used unmodified as an argument to rn_newpair() can't make it negative / overflow either.

You might be right, but I did not have enough knowledge of the code to validate this easily.

sys/net/radix.c
459

This is not logical equivalent to the original statement.

The current condition in line 459 is this:

b > (unsigned) x->rn_bit

where, as an alternative to the original diff you objected to, I'm suggesting to make explicit type casting instead of implicit:

(unsigned) b > (unsigned) x->rn_bit

Please elaborate how exactly the two conditions differ.

sys/net/radix.c
459

b > (unsigned) x->rn_bit
(unsigned) b > (unsigned) x->rn_bit

Please elaborate how exactly the two conditions differ.

b = -2 and x->rn_bit = 1 give false and true, respectively

sys/net/radix.c
459

b > (unsigned) x->rn_bit
(unsigned) b > (unsigned) x->rn_bit

Please elaborate how exactly the two conditions differ.

b = -2 and x->rn_bit = 1 give false and true, respectively

Wrong answer. Pls. compile the snippet below and report back the results.

#include <stdio.h>

int
main(int argc, char **argv)
{

int b, x;

scanf("%d", &b);
scanf("%d", &x);

printf("b: %d x: %d\n", b, x);

if (b > (unsigned) x)
        printf("%d > %d\n", b, x);

if ((unsigned) b > (unsigned) x)
        printf("%d > %d\n", b, x);

}

https://blog.regehr.org/archives/268
It's complicated.

#include <stdio.h>

int main(int argc, char **argv)
{
   int b;
   unsigned short x;

   scanf("%d", &b);
   scanf("%hd", &x);

   printf("b: %d x: %d\n", b, x);

   if (b > x)
     printf("%d > %d\n", b, x);
   else
     printf("failed\n");

   if ((unsigned) b > x)
     printf("%d > %d\n", b, x);
   else
     printf("failed\n");

}

So the (unsigned) conversion is also a size conversion (to int). This can become relevant, if somebody try to refine the types of the struct to save memory.
So yes, covert both sides is a solution.

This revision was not accepted when it landed; it landed in state Needs Revision.Apr 10 2021, 1:50 PM
This revision was automatically updated to reflect the committed changes.