| Message ID | 20240328143051.1069575-6-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | address remaining -Wtautological-constant-out-of-range-compare | expand |
On Thu, Mar 28, 2024 at 3:31 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > Clang warns about a range check in percpu_add_op() being impossible > to hit for an u8 variable: > > net/ipv4/tcp_output.c:188:3: error: result of comparison of constant -1 with expression of type 'u8' (aka 'unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare] > NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/net/ip.h:291:41: note: expanded from macro 'NET_ADD_STATS' > #define NET_ADD_STATS(net, field, adnd) SNMP_ADD_STATS((net)->mib.net_statistics, field, adnd) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/net/snmp.h:143:4: note: expanded from macro 'SNMP_ADD_STATS' > this_cpu_add(mib->mibs[field], addend) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/percpu-defs.h:509:33: note: expanded from macro 'this_cpu_add' > #define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all) > <scratch space>:187:1: note: expanded from here > this_cpu_add_8 > ^ > arch/x86/include/asm/percpu.h:326:35: note: expanded from macro 'this_cpu_add_8' > #define this_cpu_add_8(pcp, val) percpu_add_op(8, volatile, (pcp), val) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op' > ((val) == 1 || (val) == -1)) ? \ > ~~~~~ ^ ~~ > This seems like a bug in the macro or the compiler, because val is not a constant ? __builtin_constant_p(val) should return false ??? +#define percpu_add_op(size, qual, var, val) \ +do { \ + const int pao_ID__ = (__builtin_constant_p(val) && \ + ((val) == 1 || (val) == -1)) ? \ + (int)(val) : 0; \ + if (0) { \ + typeof(var) pao_tmp__; \ + pao_tmp__ = (val); \ + (void)pao_tmp__; \ + } \ + if (pao_ID__ == 1) \ + percpu_unary_op(size, qual, "inc", var); \ + else if (pao_ID__ == -1) \ + percpu_unary_op(size, qual, "dec", var); \ + else \ + percpu_to_op(size, qual, "add", var, val); \ +} while (0) + > Avoid this warning with a cast to a signed 'int'. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > net/ipv4/tcp_output.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index e3167ad96567..dbe54fceee08 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -183,7 +183,7 @@ static inline void tcp_event_ack_sent(struct sock *sk, u32 rcv_nxt) > > if (unlikely(tp->compressed_ack)) { > NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED, > - tp->compressed_ack); > + (int)tp->compressed_ack); > tp->compressed_ack = 0; > if (hrtimer_try_to_cancel(&tp->compressed_ack_timer) == 1) > __sock_put(sk); > -- > 2.39.2 >
On Thu, Mar 28, 2024, at 15:38, Eric Dumazet wrote: > On Thu, Mar 28, 2024 at 3:31 PM Arnd Bergmann <arnd@kernel.org> wrote: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op' >> ((val) == 1 || (val) == -1)) ? \ >> ~~~~~ ^ ~~ >> > > This seems like a bug in the macro or the compiler, because val is not > a constant ? > > __builtin_constant_p(val) should return false ??? > > +#define percpu_add_op(size, qual, var, val) \ > +do { \ > + const int pao_ID__ = (__builtin_constant_p(val) && \ > + ((val) == 1 || (val) == -1)) ? \ > + (int)(val) : 0; \ It looks like gcc does the same thing, with the broader and still disabled -Wtype-limits, see: https://godbolt.org/z/3EPTGx68n As far as I can tell, it does not matter that the comparison against -1 is never actually evaluated, since the warning is already printed before it simplifies the condition. This is the only such warning I got from percpu, but I guess we could also add the cast inside of the macro, such as diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h index 44958ebaf626..5923d786e67a 100644 --- a/arch/x86/include/asm/percpu.h +++ b/arch/x86/include/asm/percpu.h @@ -181,12 +181,14 @@ do { \ */ #define percpu_add_op(size, qual, var, val) \ do { \ - const int pao_ID__ = (__builtin_constant_p(val) && \ - ((val) == 1 || (val) == -1)) ? \ - (int)(val) : 0; \ + __auto_type __val = (val); \ + const int pao_ID__ = (__builtin_constant_p(__val) && \ + ((__val) == (typeof(__val))1 || \ + (__val) == (typeof(__val))-1)) ? \ + (int)(__val) : 0; \ if (0) { \ typeof(var) pao_tmp__; \ - pao_tmp__ = (val); \ + pao_tmp__ = (__val); \ (void)pao_tmp__; \ } \ if (pao_ID__ == 1) \ @@ -194,7 +196,7 @@ do { \ else if (pao_ID__ == -1) \ percpu_unary_op(size, qual, "dec", var); \ else \ - percpu_to_op(size, qual, "add", var, val); \ + percpu_to_op(size, qual, "add", var, __val); \ } while (0) #define percpu_from_op(size, qual, op, _var) \ I added a temporary variable there to avoid expanding the argument too many times. Arnd
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index e3167ad96567..dbe54fceee08 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -183,7 +183,7 @@ static inline void tcp_event_ack_sent(struct sock *sk, u32 rcv_nxt) if (unlikely(tp->compressed_ack)) { NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED, - tp->compressed_ack); + (int)tp->compressed_ack); tp->compressed_ack = 0; if (hrtimer_try_to_cancel(&tp->compressed_ack_timer) == 1) __sock_put(sk);