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

Arnd Bergmann
2024-03-28T16:39:57Z

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