[PATCH] tcp: Avoid comparison of expressions with different signedness in RTT_SET()
With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine
on x86_64):
tcp.c: In function 'tcp_update_seqack_wnd':
util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare]
40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y))
| ^
tcp_conn.h:63:26: note: in expansion of macro 'MIN'
63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN))))
| ^~~
tcp.c:1234:17: note: in expansion of macro 'RTT_SET'
1234 | RTT_SET(conn, tinfo->tcpi_rtt);
| ^~~~~~~
util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare]
40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y))
| ^~~
tcp_conn.h:63:26: note: in expansion of macro 'MIN'
63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN))))
| ^~~
tcp.c:1234:17: note: in expansion of macro 'RTT_SET'
1234 | RTT_SET(conn, tinfo->tcpi_rtt);
| ^~~~~~~
for some reason, that's not reported by gcc with glibc.
Cast the result of ilog2() to unsigned before using it, as it's always
positive the way we're using it. Should this ever break this for
whatever unlikely reason, RTT_EXP_MAX is the fallback value we want to
use anyway.
Fixes: 000601ba86da ("tcp: Adaptive interval based on RTT for socket-side acknowledgement checks")
Signed-off-by: Stefano Brivio
On Wed, Mar 04, 2026 at 05:32:32PM +0100, Stefano Brivio wrote:
With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine on x86_64):
tcp.c: In function 'tcp_update_seqack_wnd': util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~ util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^~~ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~
for some reason, that's not reported by gcc with glibc.
Cast the result of ilog2() to unsigned before using it, as it's always positive the way we're using it. Should this ever break this for whatever unlikely reason, RTT_EXP_MAX is the fallback value we want to use anyway.
Fixes: 000601ba86da ("tcp: Adaptive interval based on RTT for socket-side acknowledgement checks") Signed-off-by: Stefano Brivio
This is correct, so
Reviewed-by: David Gibson
On Thu, 5 Mar 2026 13:32:08 +1100
David Gibson
On Wed, Mar 04, 2026 at 05:32:32PM +0100, Stefano Brivio wrote:
With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine on x86_64):
tcp.c: In function 'tcp_update_seqack_wnd': util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~ util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^~~ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~
for some reason, that's not reported by gcc with glibc.
Cast the result of ilog2() to unsigned before using it, as it's always positive the way we're using it. Should this ever break this for whatever unlikely reason, RTT_EXP_MAX is the fallback value we want to use anyway.
Fixes: 000601ba86da ("tcp: Adaptive interval based on RTT for socket-side acknowledgement checks") Signed-off-by: Stefano Brivio
This is correct, so
Reviewed-by: David Gibson
It's kind of inelegant, though - ilog2() only returns a signed to report an error case, which we avoid with the MAX inside the argument. That's not a super obvious connection. I guess we could do:
(unsigned)MAX(0, ilog2(rtt / RTT_STORE_MIN))
which should be equivalent and makes it slightly more obvious that the cast is safe. Not sure if it's really an improvement, though.
Right, yes, at this point I guess it's clearer. Changed to that in v2.
This is the only user of ilog2(), so we could also consider replacing it with a version that explicitly clamps its argument to >= 1 and returns unsigned.
I thought about doing that, but on the other hand we would have a rather arithmetic function returning an obviously wrong value for some arguments, which doesn't sound great either. -- Stefano
On Fri, Mar 06, 2026 at 08:02:26AM +0100, Stefano Brivio wrote:
On Thu, 5 Mar 2026 13:32:08 +1100 David Gibson
wrote: On Wed, Mar 04, 2026 at 05:32:32PM +0100, Stefano Brivio wrote:
With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine on x86_64):
tcp.c: In function 'tcp_update_seqack_wnd': util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~ util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^~~ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~
for some reason, that's not reported by gcc with glibc.
Cast the result of ilog2() to unsigned before using it, as it's always positive the way we're using it. Should this ever break this for whatever unlikely reason, RTT_EXP_MAX is the fallback value we want to use anyway.
Fixes: 000601ba86da ("tcp: Adaptive interval based on RTT for socket-side acknowledgement checks") Signed-off-by: Stefano Brivio
This is correct, so
Reviewed-by: David Gibson
It's kind of inelegant, though - ilog2() only returns a signed to report an error case, which we avoid with the MAX inside the argument. That's not a super obvious connection. I guess we could do:
(unsigned)MAX(0, ilog2(rtt / RTT_STORE_MIN))
which should be equivalent and makes it slightly more obvious that the cast is safe. Not sure if it's really an improvement, though.
Right, yes, at this point I guess it's clearer. Changed to that in v2.
Ok.
This is the only user of ilog2(), so we could also consider replacing it with a version that explicitly clamps its argument to >= 1 and returns unsigned.
I thought about doing that, but on the other hand we would have a rather arithmetic function returning an obviously wrong value for some arguments, which doesn't sound great either.
Yeah, fair point. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
participants (2)
-
David Gibson
-
Stefano Brivio