Re: [PATCH 5/9] ipv4: tcp_output: avoid warning about NET_ADD_STATS

From: "Arnd Bergmann" <arnd@arndb.de>
To: "Eric Dumazet" <edumazet@google.com>, "Arnd Bergmann" <arnd@kernel.org>
Cc: linux-kernel@vger.kernel.org,
	"David S . Miller" <davem@davemloft.net>,
	"David Ahern" <dsahern@kernel.org>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Nathan Chancellor" <nathan@kernel.org>,
	"Nick Desaulniers" <ndesaulniers@google.com>,
	"Bill Wendling" <morbo@google.com>,
	"Justin Stitt" <justinstitt@google.com>,
	"Dmitry Safonov" <0x7f454c46@gmail.com>,
	"Neal Cardwell" <ncardwell@google.com>,
	"mfreemon@cloudflare.com" <mfreemon@cloudflare.com>,
	"Yan Zhai" <yan@cloudflare.com>, Netdev <netdev@vger.kernel.org>,
	llvm@lists.linux.dev
Subject: Re: [PATCH 5/9] ipv4: tcp_output: avoid warning about NET_ADD_STATS
Date: Thu, 28 Mar 2024 17:39:36 +0100	[thread overview]
Message-ID: <62cf1c3c-29b7-48cc-9d52-cd47a6c07aa4@app.fastmail.com> (raw)
In-Reply-To: <CANn89i+3FuKc1RsYaciNe3uQMZuJBjSmvC_ueuQ=NaFVzEnyuA@mail.gmail.com>

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
