Received-SPF: pass (google.com: domain of linux-kernel+bounces-123292-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-333-gbfea15422e-fm-20240327.001-gbfea1542
Precedence: bulk
MIME-Version: 1.0
Message-Id: <62cf1c3c-29b7-48cc-9d52-cd47a6c07aa4@app.fastmail.com>
In-Reply-To: 
 <CANn89i+3FuKc1RsYaciNe3uQMZuJBjSmvC_ueuQ=NaFVzEnyuA@mail.gmail.com>
References: <20240328143051.1069575-1-arnd@kernel.org>
 <20240328143051.1069575-6-arnd@kernel.org>
 <CANn89i+3FuKc1RsYaciNe3uQMZuJBjSmvC_ueuQ=NaFVzEnyuA@mail.gmail.com>
Date: Thu, 28 Mar 2024 17:39:36 +0100
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
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 28, 2024, at 15:38, Eric Dumazet wrote:
> On Thu, Mar 28, 2024 at 3:31=E2=80=AFPM Arnd Bergmann <arnd@kernel.org=
> wrote:
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'perc=
pu_add_op'
>>                               ((val) =3D=3D 1 || (val) =3D=3D -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__ =3D (__builtin_constant_p(val) &&          =
    \
> +                             ((val) =3D=3D 1 || (val) =3D=3D -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__ =3D (__builtin_constant_p(val) &&            =
  \
-                             ((val) =3D=3D 1 || (val) =3D=3D -1)) ?    =
        \
-                               (int)(val) : 0;                         \
+       __auto_type __val =3D (val);                                    =
  \
+       const int pao_ID__ =3D (__builtin_constant_p(__val) &&          =
  \
+                             ((__val) =3D=3D (typeof(__val))1 ||       =
    \
+                              (__val) =3D=3D (typeof(__val))-1)) ?     =
    \
+                               (int)(__val) : 0;                       \
        if (0) {                                                        \
                typeof(var) pao_tmp__;                                  \
-               pao_tmp__ =3D (val);                                    =
  \
+               pao_tmp__ =3D (__val);                                  =
  \
                (void)pao_tmp__;                                        \
        }                                                               \
        if (pao_ID__ =3D=3D 1)                                          =
    \
@@ -194,7 +196,7 @@ do {                                                =
                        \
        else if (pao_ID__ =3D=3D -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)
=20
 #define percpu_from_op(size, qual, op, _var)                           \

I added a temporary variable there to avoid expanding
the argument too many times.

       Arnd