Re: kernel: Resolve a shadow warning

Re: kernel: Resolve a shadow warning

[Andrew Morton]

> From: Mark Rustad <mark.d.rustad@intel.com>
> 
> Resolve a shadow warning in W=2 builds arising from min/max macro
> references in a parameter to a min3/max3 macro. There is no
> functional issue - the warning is benign - but simply changing
> some local variable names will eliminate it.
> 
> ...
>
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -716,22 +716,22 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  	_max1 > _max2 ? _max1 : _max2; })
>  
>  #define min3(x, y, z) ({			\
> -	typeof(x) _min1 = (x);			\
> -	typeof(y) _min2 = (y);			\
> -	typeof(z) _min3 = (z);			\
> -	(void) (&_min1 == &_min2);		\
> -	(void) (&_min1 == &_min3);		\
> -	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
> -		(_min2 < _min3 ? _min2 : _min3); })
> +	typeof(x) _min31 = (x);			\
> +	typeof(y) _min32 = (y);			\
> +	typeof(z) _min33 = (z);			\
> +	(void) (&_min31 == &_min32);		\
> +	(void) (&_min31 == &_min33);		\
> +	_min31 < _min32 ? (_min31 < _min33 ? _min31 : _min33) : \
> +		(_min32 < _min33 ? _min32 : _min33); })
>  
>  #define max3(x, y, z) ({			\
> -	typeof(x) _max1 = (x);			\
> -	typeof(y) _max2 = (y);			\
> -	typeof(z) _max3 = (z);			\
> -	(void) (&_max1 == &_max2);		\
> -	(void) (&_max1 == &_max3);		\
> -	_max1 > _max2 ? (_max1 > _max3 ? _max1 : _max3) : \
> -		(_max2 > _max3 ? _max2 : _max3); })
> +	typeof(x) _max31 = (x);			\
> +	typeof(y) _max32 = (y);			\
> +	typeof(z) _max33 = (z);			\
> +	(void) (&_max31 == &_max32);		\
> +	(void) (&_max31 == &_max33);		\
> +	_max31 > _max32 ? (_max31 > _max33 ? _max31 : _max33) : \
> +		(_max32 > _max33 ? _max32 : _max33); })
>  
>  /**
>   * min_not_zero - return the minimum that is _not_ zero, unless both are zero

I'm still sitting on the below patch.  It's stalled because I have a
note that David potentially found issues with it.  But on rechecking,
that appears to be stale, or not serious enough to prevent inclusion.

I can't (be bothered to) check whether this patch fixes the warnings
because your changelog didn't tell me how to trigger the warnings (bad
changelog).  But it might fix them!  Can you please test it?



From: Michal Nazarewicz <mina86@mina86.com>
Subject: include/linux/kernel.h: rewrite min3, max3 and clamp using min and max

It appears that gcc is better at optimising a double call to min and max
rather than open coded min3 and max3.  This can be observed here:

    $ cat min-max.c
    #define min(x, y) ({				\
    	typeof(x) _min1 = (x);			\
    	typeof(y) _min2 = (y);			\
    	(void) (&_min1 == &_min2);		\
    	_min1 < _min2 ? _min1 : _min2; })
    #define min3(x, y, z) ({			\
    	typeof(x) _min1 = (x);			\
    	typeof(y) _min2 = (y);			\
    	typeof(z) _min3 = (z);			\
    	(void) (&_min1 == &_min2);		\
    	(void) (&_min1 == &_min3);		\
    	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
    		(_min2 < _min3 ? _min2 : _min3); })

    int fmin3(int x, int y, int z) { return min3(x, y, z); }
    int fmin2(int x, int y, int z) { return min(min(x, y), z); }

    $ gcc -O2 -o min-max.s -S min-max.c; cat min-max.s
    	.file	"min-max.c"
    	.text
    	.p2align 4,,15
    	.globl	fmin3
    	.type	fmin3, @function
    fmin3:
    .LFB0:
    	.cfi_startproc
    	cmpl	%esi, %edi
    	jl	.L5
    	cmpl	%esi, %edx
    	movl	%esi, %eax
    	cmovle	%edx, %eax
    	ret
    	.p2align 4,,10
    	.p2align 3
    .L5:
    	cmpl	%edi, %edx
    	movl	%edi, %eax
    	cmovle	%edx, %eax
    	ret
    	.cfi_endproc
    .LFE0:
    	.size	fmin3, .-fmin3
    	.p2align 4,,15
    	.globl	fmin2
    	.type	fmin2, @function
    fmin2:
    .LFB1:
    	.cfi_startproc
    	cmpl	%edi, %esi
    	movl	%edx, %eax
    	cmovle	%esi, %edi
    	cmpl	%edx, %edi
    	cmovle	%edi, %eax
    	ret
    	.cfi_endproc
    .LFE1:
    	.size	fmin2, .-fmin2
    	.ident	"GCC: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3"
    	.section	.note.GNU-stack,"",@progbits

fmin3 function, which uses open-coded min3 macro, is compiled into total
of ten instructions including a conditional branch, whereas fmin2
function, which uses two calls to min2 macro, is compiled into six
instructions with no branches.

Similarly, open-coded clamp produces the same code as clamp using min and
max macros, but the latter is much shorter:

    $ cat clamp.c
    #define clamp(val, min, max) ({			\
    	typeof(val) __val = (val);		\
    	typeof(min) __min = (min);		\
    	typeof(max) __max = (max);		\
    	(void) (&__val == &__min);		\
    	(void) (&__val == &__max);		\
    	__val = __val < __min ? __min: __val;	\
    	__val > __max ? __max: __val; })
    #define min(x, y) ({				\
    	typeof(x) _min1 = (x);			\
    	typeof(y) _min2 = (y);			\
    	(void) (&_min1 == &_min2);		\
    	_min1 < _min2 ? _min1 : _min2; })
    #define max(x, y) ({				\
    	typeof(x) _max1 = (x);			\
    	typeof(y) _max2 = (y);			\
    	(void) (&_max1 == &_max2);		\
    	_max1 > _max2 ? _max1 : _max2; })

    int fclamp(int v, int min, int max) { return clamp(v, min, max); }
    int fclampmm(int v, int min, int max) { return min(max(v, min), max); }

    $ gcc -O2 -o clamp.s -S clamp.c; cat clamp.s
    	.file	"clamp.c"
    	.text
    	.p2align 4,,15
    	.globl	fclamp
    	.type	fclamp, @function
    fclamp:
    .LFB0:
    	.cfi_startproc
    	cmpl	%edi, %esi
    	movl	%edx, %eax
    	cmovge	%esi, %edi
    	cmpl	%edx, %edi
    	cmovle	%edi, %eax
    	ret
    	.cfi_endproc
    .LFE0:
    	.size	fclamp, .-fclamp
    	.p2align 4,,15
    	.globl	fclampmm
    	.type	fclampmm, @function
    fclampmm:
    .LFB1:
    	.cfi_startproc
    	cmpl	%edi, %esi
    	cmovge	%esi, %edi
    	cmpl	%edi, %edx
    	movl	%edi, %eax
    	cmovle	%edx, %eax
    	ret
    	.cfi_endproc
    .LFE1:
    	.size	fclampmm, .-fclampmm
    	.ident	"GCC: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3"
    	.section	.note.GNU-stack,"",@progbits

    Linux mpn-glaptop 3.13.0-29-generic #53~precise1-Ubuntu SMP Wed Jun 4 22:06:25 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
    gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
    Copyright (C) 2011 Free Software Foundation, Inc.
    This is free software; see the source for copying conditions.  There is NO
    warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
    
    -rwx------ 1 mpn eng 51224656 Jun 17 14:15 vmlinux.before
    -rwx------ 1 mpn eng 51224608 Jun 17 13:57 vmlinux.after

48 bytes reduction.  The do_fault_around was a few instruction shorter
and as far as I can tell saved 12 bytes on the stack, i.e.:

    $ grep -e rsp -e pop -e push do_fault_around.*
    do_fault_around.before.s:push   %rbp
    do_fault_around.before.s:mov    %rsp,%rbp
    do_fault_around.before.s:push   %r13
    do_fault_around.before.s:push   %r12
    do_fault_around.before.s:push   %rbx
    do_fault_around.before.s:sub    $0x38,%rsp
    do_fault_around.before.s:add    $0x38,%rsp
    do_fault_around.before.s:pop    %rbx
    do_fault_around.before.s:pop    %r12
    do_fault_around.before.s:pop    %r13
    do_fault_around.before.s:pop    %rbp

    do_fault_around.after.s:push   %rbp
    do_fault_around.after.s:mov    %rsp,%rbp
    do_fault_around.after.s:push   %r12
    do_fault_around.after.s:push   %rbx
    do_fault_around.after.s:sub    $0x30,%rsp
    do_fault_around.after.s:add    $0x30,%rsp
    do_fault_around.after.s:pop    %rbx
    do_fault_around.after.s:pop    %r12
    do_fault_around.after.s:pop    %rbp

or here side-by-side:

    Before                    After
    push   %rbp               push   %rbp          
    mov    %rsp,%rbp          mov    %rsp,%rbp     
    push   %r13                                    
    push   %r12               push   %r12          
    push   %rbx               push   %rbx          
    sub    $0x38,%rsp         sub    $0x30,%rsp    
    add    $0x38,%rsp         add    $0x30,%rsp    
    pop    %rbx               pop    %rbx          
    pop    %r12               pop    %r12          
    pop    %r13                                    
    pop    %rbp               pop    %rbp          

There are also fewer branches:

    $ grep ^j do_fault_around.*
    do_fault_around.before.s:jae    ffffffff812079b7
    do_fault_around.before.s:jmp    ffffffff812079c5
    do_fault_around.before.s:jmp    ffffffff81207a14
    do_fault_around.before.s:ja     ffffffff812079f9
    do_fault_around.before.s:jb     ffffffff81207a10
    do_fault_around.before.s:jmp    ffffffff81207a63
    do_fault_around.before.s:jne    ffffffff812079df
    
    do_fault_around.after.s:jmp    ffffffff812079fd
    do_fault_around.after.s:ja     ffffffff812079e2
    do_fault_around.after.s:jb     ffffffff812079f9
    do_fault_around.after.s:jmp    ffffffff81207a4c
    do_fault_around.after.s:jne    ffffffff812079c8

And here's with allyesconfig on a different machine:

    $ uname -a; gcc --version; ls -l vmlinux.*
    Linux erwin 3.14.7-mn #54 SMP Sun Jun 15 11:25:08 CEST 2014 x86_64 AMD Phenom(tm) II X3 710 Processor AuthenticAMD GNU/Linux
    gcc (GCC) 4.8.3
    Copyright (C) 2013 Free Software Foundation, Inc.
    This is free software; see the source for copying conditions.  There is NO
    warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

    -rwx------ 1 mpn eng 437027411 Jun 20 16:04 vmlinux.before
    -rwx------ 1 mpn eng 437026881 Jun 20 15:30 vmlinux.after

530 bytes reduction.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Hagen Paul Pfeifer <hagen@jauu.net>
Cc: David Rientjes <rientjes@google.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/kernel.h |   32 +++++---------------------------
 1 file changed, 5 insertions(+), 27 deletions(-)

diff -puN include/linux/kernel.h~include-kernelh-rewrite-min3-max3-and-clamp-using-min-and-max include/linux/kernel.h
--- a/include/linux/kernel.h~include-kernelh-rewrite-min3-max3-and-clamp-using-min-and-max
+++ a/include/linux/kernel.h
@@ -715,23 +715,8 @@ static inline void ftrace_dump(enum ftra
 	(void) (&_max1 == &_max2);		\
 	_max1 > _max2 ? _max1 : _max2; })
 
-#define min3(x, y, z) ({			\
-	typeof(x) _min1 = (x);			\
-	typeof(y) _min2 = (y);			\
-	typeof(z) _min3 = (z);			\
-	(void) (&_min1 == &_min2);		\
-	(void) (&_min1 == &_min3);		\
-	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
-		(_min2 < _min3 ? _min2 : _min3); })
-
-#define max3(x, y, z) ({			\
-	typeof(x) _max1 = (x);			\
-	typeof(y) _max2 = (y);			\
-	typeof(z) _max3 = (z);			\
-	(void) (&_max1 == &_max2);		\
-	(void) (&_max1 == &_max3);		\
-	_max1 > _max2 ? (_max1 > _max3 ? _max1 : _max3) : \
-		(_max2 > _max3 ? _max2 : _max3); })
+#define min3(x, y, z) min((typeof(x))min(x, y), z)
+#define max3(x, y, z) max((typeof(x))max(x, y), z)
 
 /**
  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
@@ -746,20 +731,13 @@ static inline void ftrace_dump(enum ftra
 /**
  * clamp - return a value clamped to a given range with strict typechecking
  * @val: current value
- * @min: minimum allowable value
- * @max: maximum allowable value
+ * @lo: lowest allowable value
+ * @hi: highest allowable value
  *
  * This macro does strict typechecking of min/max to make sure they are of the
  * same type as val.  See the unnecessary pointer comparisons.
  */
-#define clamp(val, min, max) ({			\
-	typeof(val) __val = (val);		\
-	typeof(min) __min = (min);		\
-	typeof(max) __max = (max);		\
-	(void) (&__val == &__min);		\
-	(void) (&__val == &__max);		\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
+#define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)
 
 /*
  * ..and if you can't take the strict
_

Re: kernel: Resolve a shadow warning

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 13094 bytes --]

On Sep 4, 2014, at 2:38 PM, Andrew Morton <akpm@linux-foundation.org> wrote:

>> From: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> Resolve a shadow warning in W=2 builds arising from min/max macro
>> references in a parameter to a min3/max3 macro. There is no
>> functional issue - the warning is benign - but simply changing
>> some local variable names will eliminate it.
>> 
>> ...
>> 
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -716,22 +716,22 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>> 	_max1 > _max2 ? _max1 : _max2; })
>> 
>> #define min3(x, y, z) ({			\
>> -	typeof(x) _min1 = (x);			\
>> -	typeof(y) _min2 = (y);			\
>> -	typeof(z) _min3 = (z);			\
>> -	(void) (&_min1 == &_min2);		\
>> -	(void) (&_min1 == &_min3);		\
>> -	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
>> -		(_min2 < _min3 ? _min2 : _min3); })
>> +	typeof(x) _min31 = (x);			\
>> +	typeof(y) _min32 = (y);			\
>> +	typeof(z) _min33 = (z);			\
>> +	(void) (&_min31 == &_min32);		\
>> +	(void) (&_min31 == &_min33);		\
>> +	_min31 < _min32 ? (_min31 < _min33 ? _min31 : _min33) : \
>> +		(_min32 < _min33 ? _min32 : _min33); })
>> 
>> #define max3(x, y, z) ({			\
>> -	typeof(x) _max1 = (x);			\
>> -	typeof(y) _max2 = (y);			\
>> -	typeof(z) _max3 = (z);			\
>> -	(void) (&_max1 == &_max2);		\
>> -	(void) (&_max1 == &_max3);		\
>> -	_max1 > _max2 ? (_max1 > _max3 ? _max1 : _max3) : \
>> -		(_max2 > _max3 ? _max2 : _max3); })
>> +	typeof(x) _max31 = (x);			\
>> +	typeof(y) _max32 = (y);			\
>> +	typeof(z) _max33 = (z);			\
>> +	(void) (&_max31 == &_max32);		\
>> +	(void) (&_max31 == &_max33);		\
>> +	_max31 > _max32 ? (_max31 > _max33 ? _max31 : _max33) : \
>> +		(_max32 > _max33 ? _max32 : _max33); })
>> 
>> /**
>>  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
> 
> I'm still sitting on the below patch.  It's stalled because I have a
> note that David potentially found issues with it.  But on rechecking,
> that appears to be stale, or not serious enough to prevent inclusion.
> 
> I can't (be bothered to) check whether this patch fixes the warnings
> because your changelog didn't tell me how to trigger the warnings (bad
> changelog).  But it might fix them!  Can you please test it?

Actually it does. W=2 builds get warnings when a min/max macro is used as a parameter to min3/max3. If you move to the min3/max3 that makes nested calls to min/max, every reference to min3/max3 will generate a warning in W=2 builds no matter what. It is interesting that the compiler optimizes that better - I hadn't looked at that.

Not wanting to argue for poorer code, lets forget about the warnings for now. These particular shadow warnings are pretty trivial. I'll consider revisiting it later. And next time I'll check the code generation.

> From: Michal Nazarewicz <mina86@mina86.com>
> Subject: include/linux/kernel.h: rewrite min3, max3 and clamp using min and max
> 
> It appears that gcc is better at optimising a double call to min and max
> rather than open coded min3 and max3.  This can be observed here:
> 
>    $ cat min-max.c
>    #define min(x, y) ({				\
>    	typeof(x) _min1 = (x);			\
>    	typeof(y) _min2 = (y);			\
>    	(void) (&_min1 == &_min2);		\
>    	_min1 < _min2 ? _min1 : _min2; })
>    #define min3(x, y, z) ({			\
>    	typeof(x) _min1 = (x);			\
>    	typeof(y) _min2 = (y);			\
>    	typeof(z) _min3 = (z);			\
>    	(void) (&_min1 == &_min2);		\
>    	(void) (&_min1 == &_min3);		\
>    	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
>    		(_min2 < _min3 ? _min2 : _min3); })
> 
>    int fmin3(int x, int y, int z) { return min3(x, y, z); }
>    int fmin2(int x, int y, int z) { return min(min(x, y), z); }
> 
>    $ gcc -O2 -o min-max.s -S min-max.c; cat min-max.s
>    	.file	"min-max.c"
>    	.text
>    	.p2align 4,,15
>    	.globl	fmin3
>    	.type	fmin3, @function
>    fmin3:
>    .LFB0:
>    	.cfi_startproc
>    	cmpl	%esi, %edi
>    	jl	.L5
>    	cmpl	%esi, %edx
>    	movl	%esi, %eax
>    	cmovle	%edx, %eax
>    	ret
>    	.p2align 4,,10
>    	.p2align 3
>    .L5:
>    	cmpl	%edi, %edx
>    	movl	%edi, %eax
>    	cmovle	%edx, %eax
>    	ret
>    	.cfi_endproc
>    .LFE0:
>    	.size	fmin3, .-fmin3
>    	.p2align 4,,15
>    	.globl	fmin2
>    	.type	fmin2, @function
>    fmin2:
>    .LFB1:
>    	.cfi_startproc
>    	cmpl	%edi, %esi
>    	movl	%edx, %eax
>    	cmovle	%esi, %edi
>    	cmpl	%edx, %edi
>    	cmovle	%edi, %eax
>    	ret
>    	.cfi_endproc
>    .LFE1:
>    	.size	fmin2, .-fmin2
>    	.ident	"GCC: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3"
>    	.section	.note.GNU-stack,"",@progbits
> 
> fmin3 function, which uses open-coded min3 macro, is compiled into total
> of ten instructions including a conditional branch, whereas fmin2
> function, which uses two calls to min2 macro, is compiled into six
> instructions with no branches.
> 
> Similarly, open-coded clamp produces the same code as clamp using min and
> max macros, but the latter is much shorter:
> 
>    $ cat clamp.c
>    #define clamp(val, min, max) ({			\
>    	typeof(val) __val = (val);		\
>    	typeof(min) __min = (min);		\
>    	typeof(max) __max = (max);		\
>    	(void) (&__val == &__min);		\
>    	(void) (&__val == &__max);		\
>    	__val = __val < __min ? __min: __val;	\
>    	__val > __max ? __max: __val; })
>    #define min(x, y) ({				\
>    	typeof(x) _min1 = (x);			\
>    	typeof(y) _min2 = (y);			\
>    	(void) (&_min1 == &_min2);		\
>    	_min1 < _min2 ? _min1 : _min2; })
>    #define max(x, y) ({				\
>    	typeof(x) _max1 = (x);			\
>    	typeof(y) _max2 = (y);			\
>    	(void) (&_max1 == &_max2);		\
>    	_max1 > _max2 ? _max1 : _max2; })
> 
>    int fclamp(int v, int min, int max) { return clamp(v, min, max); }
>    int fclampmm(int v, int min, int max) { return min(max(v, min), max); }
> 
>    $ gcc -O2 -o clamp.s -S clamp.c; cat clamp.s
>    	.file	"clamp.c"
>    	.text
>    	.p2align 4,,15
>    	.globl	fclamp
>    	.type	fclamp, @function
>    fclamp:
>    .LFB0:
>    	.cfi_startproc
>    	cmpl	%edi, %esi
>    	movl	%edx, %eax
>    	cmovge	%esi, %edi
>    	cmpl	%edx, %edi
>    	cmovle	%edi, %eax
>    	ret
>    	.cfi_endproc
>    .LFE0:
>    	.size	fclamp, .-fclamp
>    	.p2align 4,,15
>    	.globl	fclampmm
>    	.type	fclampmm, @function
>    fclampmm:
>    .LFB1:
>    	.cfi_startproc
>    	cmpl	%edi, %esi
>    	cmovge	%esi, %edi
>    	cmpl	%edi, %edx
>    	movl	%edi, %eax
>    	cmovle	%edx, %eax
>    	ret
>    	.cfi_endproc
>    .LFE1:
>    	.size	fclampmm, .-fclampmm
>    	.ident	"GCC: (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3"
>    	.section	.note.GNU-stack,"",@progbits
> 
>    Linux mpn-glaptop 3.13.0-29-generic #53~precise1-Ubuntu SMP Wed Jun 4 22:06:25 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
>    gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
>    Copyright (C) 2011 Free Software Foundation, Inc.
>    This is free software; see the source for copying conditions.  There is NO
>    warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
>    -rwx------ 1 mpn eng 51224656 Jun 17 14:15 vmlinux.before
>    -rwx------ 1 mpn eng 51224608 Jun 17 13:57 vmlinux.after
> 
> 48 bytes reduction.  The do_fault_around was a few instruction shorter
> and as far as I can tell saved 12 bytes on the stack, i.e.:
> 
>    $ grep -e rsp -e pop -e push do_fault_around.*
>    do_fault_around.before.s:push   %rbp
>    do_fault_around.before.s:mov    %rsp,%rbp
>    do_fault_around.before.s:push   %r13
>    do_fault_around.before.s:push   %r12
>    do_fault_around.before.s:push   %rbx
>    do_fault_around.before.s:sub    $0x38,%rsp
>    do_fault_around.before.s:add    $0x38,%rsp
>    do_fault_around.before.s:pop    %rbx
>    do_fault_around.before.s:pop    %r12
>    do_fault_around.before.s:pop    %r13
>    do_fault_around.before.s:pop    %rbp
> 
>    do_fault_around.after.s:push   %rbp
>    do_fault_around.after.s:mov    %rsp,%rbp
>    do_fault_around.after.s:push   %r12
>    do_fault_around.after.s:push   %rbx
>    do_fault_around.after.s:sub    $0x30,%rsp
>    do_fault_around.after.s:add    $0x30,%rsp
>    do_fault_around.after.s:pop    %rbx
>    do_fault_around.after.s:pop    %r12
>    do_fault_around.after.s:pop    %rbp
> 
> or here side-by-side:
> 
>    Before                    After
>    push   %rbp               push   %rbp          
>    mov    %rsp,%rbp          mov    %rsp,%rbp     
>    push   %r13                                    
>    push   %r12               push   %r12          
>    push   %rbx               push   %rbx          
>    sub    $0x38,%rsp         sub    $0x30,%rsp    
>    add    $0x38,%rsp         add    $0x30,%rsp    
>    pop    %rbx               pop    %rbx          
>    pop    %r12               pop    %r12          
>    pop    %r13                                    
>    pop    %rbp               pop    %rbp          
> 
> There are also fewer branches:
> 
>    $ grep ^j do_fault_around.*
>    do_fault_around.before.s:jae    ffffffff812079b7
>    do_fault_around.before.s:jmp    ffffffff812079c5
>    do_fault_around.before.s:jmp    ffffffff81207a14
>    do_fault_around.before.s:ja     ffffffff812079f9
>    do_fault_around.before.s:jb     ffffffff81207a10
>    do_fault_around.before.s:jmp    ffffffff81207a63
>    do_fault_around.before.s:jne    ffffffff812079df
> 
>    do_fault_around.after.s:jmp    ffffffff812079fd
>    do_fault_around.after.s:ja     ffffffff812079e2
>    do_fault_around.after.s:jb     ffffffff812079f9
>    do_fault_around.after.s:jmp    ffffffff81207a4c
>    do_fault_around.after.s:jne    ffffffff812079c8
> 
> And here's with allyesconfig on a different machine:
> 
>    $ uname -a; gcc --version; ls -l vmlinux.*
>    Linux erwin 3.14.7-mn #54 SMP Sun Jun 15 11:25:08 CEST 2014 x86_64 AMD Phenom(tm) II X3 710 Processor AuthenticAMD GNU/Linux
>    gcc (GCC) 4.8.3
>    Copyright (C) 2013 Free Software Foundation, Inc.
>    This is free software; see the source for copying conditions.  There is NO
>    warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
>    -rwx------ 1 mpn eng 437027411 Jun 20 16:04 vmlinux.before
>    -rwx------ 1 mpn eng 437026881 Jun 20 15:30 vmlinux.after
> 
> 530 bytes reduction.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
> Acked-by: Steven Rostedt <rostedt@goodmis.org>
> Cc: Hagen Paul Pfeifer <hagen@jauu.net>
> Cc: David Rientjes <rientjes@google.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
> include/linux/kernel.h |   32 +++++---------------------------
> 1 file changed, 5 insertions(+), 27 deletions(-)
> 
> diff -puN include/linux/kernel.h~include-kernelh-rewrite-min3-max3-and-clamp-using-min-and-max include/linux/kernel.h
> --- a/include/linux/kernel.h~include-kernelh-rewrite-min3-max3-and-clamp-using-min-and-max
> +++ a/include/linux/kernel.h
> @@ -715,23 +715,8 @@ static inline void ftrace_dump(enum ftra
> 	(void) (&_max1 == &_max2);		\
> 	_max1 > _max2 ? _max1 : _max2; })
> 
> -#define min3(x, y, z) ({			\
> -	typeof(x) _min1 = (x);			\
> -	typeof(y) _min2 = (y);			\
> -	typeof(z) _min3 = (z);			\
> -	(void) (&_min1 == &_min2);		\
> -	(void) (&_min1 == &_min3);		\
> -	_min1 < _min2 ? (_min1 < _min3 ? _min1 : _min3) : \
> -		(_min2 < _min3 ? _min2 : _min3); })
> -
> -#define max3(x, y, z) ({			\
> -	typeof(x) _max1 = (x);			\
> -	typeof(y) _max2 = (y);			\
> -	typeof(z) _max3 = (z);			\
> -	(void) (&_max1 == &_max2);		\
> -	(void) (&_max1 == &_max3);		\
> -	_max1 > _max2 ? (_max1 > _max3 ? _max1 : _max3) : \
> -		(_max2 > _max3 ? _max2 : _max3); })
> +#define min3(x, y, z) min((typeof(x))min(x, y), z)
> +#define max3(x, y, z) max((typeof(x))max(x, y), z)
> 
> /**
>  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
> @@ -746,20 +731,13 @@ static inline void ftrace_dump(enum ftra
> /**
>  * clamp - return a value clamped to a given range with strict typechecking
>  * @val: current value
> - * @min: minimum allowable value
> - * @max: maximum allowable value
> + * @lo: lowest allowable value
> + * @hi: highest allowable value
>  *
>  * This macro does strict typechecking of min/max to make sure they are of the
>  * same type as val.  See the unnecessary pointer comparisons.
>  */
> -#define clamp(val, min, max) ({			\
> -	typeof(val) __val = (val);		\
> -	typeof(min) __min = (min);		\
> -	typeof(max) __max = (max);		\
> -	(void) (&__val == &__min);		\
> -	(void) (&__val == &__max);		\
> -	__val = __val < __min ? __min: __val;	\
> -	__val > __max ? __max: __val; })
> +#define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)
> 
> /*
>  * ..and if you can't take the strict
> _
> 

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

[PATCH 1/2] include: kernel.h: deduplicate code implementing min*, max* and clamp* macros

[Michal Nazarewicz]

Apart from temporary variable names, min and max macros have only one
difference -- the operator used to compare the arguments.  Furthermore,
min and min_t differ only in the type used for the temporary variables.
Same with max and max_t.  All of those macros can be trivially folded
into a single generic implementation which takes the operator used and
type of the arguments.

Furthermore, similarly to how clamp uses min(max(…), …) call, clamp_t
can be simplified to do the same but with min_t(max_t(…), …).  And
finally, clamp_val is just a special case of clamp_t where type is
typeof(val).

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/linux/kernel.h | 54 ++++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 37 deletions(-)

This has been compile tested only.

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0588802..8e24054 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -699,17 +699,14 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * strict type-checking.. See the
  * "unnecessary" pointer comparison.
  */
-#define min(x, y) ({				\
-	typeof(x) _min1 = (x);			\
-	typeof(y) _min2 = (y);			\
-	(void) (&_min1 == &_min2);		\
-	_min1 < _min2 ? _min1 : _min2; })
-
-#define max(x, y) ({				\
-	typeof(x) _max1 = (x);			\
-	typeof(y) _max2 = (y);			\
-	(void) (&_max1 == &_max2);		\
-	_max1 > _max2 ? _max1 : _max2; })
+#define _min_max(op, tx, x, ty, y) ({	\
+	tx _xx = (x);			\
+	ty _yy = (y);			\
+	(void) (&_xx == &_yy);		\
+	_xx op _yy ? _xx : _yy; })
+
+#define min(x, y) _min_max(<, typeof(x), x, typeof(y), y)
+#define max(x, y) _min_max(>, typeof(x), x, typeof(y), y)
 
 #define min3(x, y, z) min((typeof(x))min(x, y), z)
 #define max3(x, y, z) max((typeof(x))max(x, y), z)
@@ -730,7 +727,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @lo: lowest allowable value
  * @hi: highest allowable value
  *
- * This macro does strict typechecking of min/max to make sure they are of the
+ * This macro does strict typechecking of lo/hi to make sure they are of the
  * same type as val.  See the unnecessary pointer comparisons.
  */
 #define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)
@@ -741,50 +738,33 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  *
  * Or not use min/max/clamp at all, of course.
  */
-#define min_t(type, x, y) ({			\
-	type __min1 = (x);			\
-	type __min2 = (y);			\
-	__min1 < __min2 ? __min1: __min2; })
-
-#define max_t(type, x, y) ({			\
-	type __max1 = (x);			\
-	type __max2 = (y);			\
-	__max1 > __max2 ? __max1: __max2; })
+#define min_t(type, x, y) _min_max(<, type, x, type, y)
+#define max_t(type, x, y) _min_max(>, type, x, type, y)
 
 /**
  * clamp_t - return a value clamped to a given range using a given type
  * @type: the type of variable to use
  * @val: current value
- * @min: minimum allowable value
- * @max: maximum allowable value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
  *
  * This macro does no typechecking and uses temporary variables of type
  * 'type' to make all the comparisons.
  */
-#define clamp_t(type, val, min, max) ({		\
-	type __val = (val);			\
-	type __min = (min);			\
-	type __max = (max);			\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
+#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
 
 /**
  * clamp_val - return a value clamped to a given range using val's type
  * @val: current value
- * @min: minimum allowable value
- * @max: maximum allowable value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
  *
  * This macro does no typechecking and uses temporary variables of whatever
  * type the input argument 'val' is.  This is useful when val is an unsigned
  * type and min and max are literals that will otherwise be assigned a signed
  * integer type.
  */
-#define clamp_val(val, min, max) ({		\
-	typeof(val) __val = (val);		\
-	typeof(val) __min = (min);		\
-	typeof(val) __max = (max);		\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
+#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
 
 
 /*
-- 
2.1.0.rc2.206.gedb03e5

[PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Michal Nazarewicz]

Because min and max macros use the same variable names no matter
how many times they are called (or how deep the nesting of their
calls), each time min or max calls are nested, the same variables
are declared.  This is especially noisy after min3 and max3 have
been changed to nest min/max calls.

Using __COUNTER__ solves the problem since each variable will get
a unique number aadded to it.  The code will still work even if
the compiler does not support __COUNTER__, but then the protection
from shadow warning won't work.

The same applies to min_t and max_t macros.

Reported-by: Rustad, Mark D <mark.d.rustad@intel.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/linux/kernel.h | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

On Thu, Sep 04 2014, "Rustad, Mark D" <mark.d.rustad@intel.com> wrote:
> W=2 builds get warnings when a min/max macro is used as a parameter to
> min3/max3. If you move to the min3/max3 that makes nested calls to
> min/max, every reference to min3/max3 will generate a warning in W=2
> builds no matter what.

This gets rid of sparse warnings.  Compile tested only.

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 8e24054..b7e4d6f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
 /*
+ * Preprocessor magic generating unique identifiers to avoid -Wshadow warnings
+ * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
+ * comparison operator; tx (ty) is type of the first (second) argument,
+ * xx (yy) is name of a temporary variable to hold the first (second) argument,
+ * and x (y) is the first (second) argument.
+ */
+#define _min_max_var(cnt, base) _mm_ ## cnt ## base
+#define _min_max__(op, tx, xx, x, ty, yy, y) ({		\
+	tx xx = (x);					\
+	ty yy = (y);					\
+	(void) (&xx == &yy);				\
+	xx op yy ? xx : yy; })
+#define _min_max_(cnt, op, tx, x, ty, y)		\
+	_min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
+#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)
+
+/*
  * min()/max()/clamp() macros that also do
  * strict type-checking.. See the
  * "unnecessary" pointer comparison.
  */
-#define _min_max(op, tx, x, ty, y) ({	\
-	tx _xx = (x);			\
-	ty _yy = (y);			\
-	(void) (&_xx == &_yy);		\
-	_xx op _yy ? _xx : _yy; })
 
 #define min(x, y) _min_max(<, typeof(x), x, typeof(y), y)
 #define max(x, y) _min_max(>, typeof(x), x, typeof(y), y)
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Andrew Morton]

On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz <mina86@mina86.com> wrote:

> Because min and max macros use the same variable names no matter
> how many times they are called (or how deep the nesting of their
> calls), each time min or max calls are nested, the same variables
> are declared.  This is especially noisy after min3 and max3 have
> been changed to nest min/max calls.
> 
> Using __COUNTER__ solves the problem since each variable will get
> a unique number aadded to it.  The code will still work even if
> the compiler does not support __COUNTER__, but then the protection
> from shadow warning won't work.
> 
> The same applies to min_t and max_t macros.
> 
> ...
>
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  #endif /* CONFIG_TRACING */
>  
>  /*
> + * Preprocessor magic generating unique identifiers to avoid -Wshadow warnings
> + * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
> + * comparison operator; tx (ty) is type of the first (second) argument,
> + * xx (yy) is name of a temporary variable to hold the first (second) argument,
> + * and x (y) is the first (second) argument.
> + */
> +#define _min_max_var(cnt, base) _mm_ ## cnt ## base
> +#define _min_max__(op, tx, xx, x, ty, yy, y) ({		\
> +	tx xx = (x);					\
> +	ty yy = (y);					\
> +	(void) (&xx == &yy);				\
> +	xx op yy ? xx : yy; })
> +#define _min_max_(cnt, op, tx, x, ty, y)		\
> +	_min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
> +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)

The fact that __COUNTER__ is used in compiler-gcc4.h but not in
compiler-gcc3.h makes me suspicious about its availability?

I do think that [1/2] made the code significantly worse-looking and
this one is getting crazy.  How useful is W=2 anyway?  Has anyone found
a bug using it?  The number of warnings in default builds is already way
too high :(

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Michal Nazarewicz]

On Fri, Sep 12 2014, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz <mina86@mina86.com> wrote:
>
>> Because min and max macros use the same variable names no matter
>> how many times they are called (or how deep the nesting of their
>> calls), each time min or max calls are nested, the same variables
>> are declared.  This is especially noisy after min3 and max3 have
>> been changed to nest min/max calls.
>> 
>> Using __COUNTER__ solves the problem since each variable will get
>> a unique number aadded to it.  The code will still work even if
>> the compiler does not support __COUNTER__, but then the protection
>> from shadow warning won't work.
>> 
>> The same applies to min_t and max_t macros.
>> 
>> ...
>>
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>>  #endif /* CONFIG_TRACING */
>>  
>>  /*
>> + * Preprocessor magic generating unique identifiers to avoid -Wshadow warnings
>> + * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
>> + * comparison operator; tx (ty) is type of the first (second) argument,
>> + * xx (yy) is name of a temporary variable to hold the first (second) argument,
>> + * and x (y) is the first (second) argument.
>> + */
>> +#define _min_max_var(cnt, base) _mm_ ## cnt ## base
>> +#define _min_max__(op, tx, xx, x, ty, yy, y) ({		\
>> +	tx xx = (x);					\
>> +	ty yy = (y);					\
>> +	(void) (&xx == &yy);				\
>> +	xx op yy ? xx : yy; })
>> +#define _min_max_(cnt, op, tx, x, ty, y)		\
>> +	_min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
>> +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)
>
> The fact that __COUNTER__ is used in compiler-gcc4.h but not in
> compiler-gcc3.h makes me suspicious about its availability?

http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01579.html so looks like it
has 7 years.  But as the commit message says, the code will still work,
even w/o working __COUNTER__.

> I do think that [1/2] made the code significantly worse-looking

Oh?  I actually thought [1/2] makes it nicer by having a single place
where the min/max logic is implemented.

> and this one is getting crazy.  How useful is W=2 anyway?

I actually do agree with that.  I didn't have high hopes about getting
this patch accepted, but wanted to send it out to show that it can be
done, if it's really deemed useful.

> Has anyone found a bug using it?  The number of warnings in default
> builds is already way too high :(

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 5518 bytes --]

On Sep 12, 2014, at 3:40 PM, Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz <mina86@mina86.com> wrote:
> 
>> Because min and max macros use the same variable names no matter
>> how many times they are called (or how deep the nesting of their
>> calls), each time min or max calls are nested, the same variables
>> are declared.  This is especially noisy after min3 and max3 have
>> been changed to nest min/max calls.
>> 
>> Using __COUNTER__ solves the problem since each variable will get
>> a unique number aadded to it.  The code will still work even if
>> the compiler does not support __COUNTER__, but then the protection
>> from shadow warning won't work.
>> 
>> The same applies to min_t and max_t macros.
>> 
>> ...
>> 
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>> #endif /* CONFIG_TRACING */
>> 
>> /*
>> + * Preprocessor magic generating unique identifiers to avoid -Wshadow warnings
>> + * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
>> + * comparison operator; tx (ty) is type of the first (second) argument,
>> + * xx (yy) is name of a temporary variable to hold the first (second) argument,
>> + * and x (y) is the first (second) argument.
>> + */
>> +#define _min_max_var(cnt, base) _mm_ ## cnt ## base
>> +#define _min_max__(op, tx, xx, x, ty, yy, y) ({		\
>> +	tx xx = (x);					\
>> +	ty yy = (y);					\
>> +	(void) (&xx == &yy);				\
>> +	xx op yy ? xx : yy; })
>> +#define _min_max_(cnt, op, tx, x, ty, y)		\
>> +	_min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
>> +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)
> 
> The fact that __COUNTER__ is used in compiler-gcc4.h but not in
> compiler-gcc3.h makes me suspicious about its availability?

AFAICT not having __COUNTER__ simply means that the shadow warnings will appear in W=2 builds,
so it is no worse off than before. That is, __COUNTER__ will simply expand as __COUNTER__. No
error will result from this kind of use.

> I do think that [1/2] made the code significantly worse-looking and
> this one is getting crazy.

It is a little crazy, but I find that I would feel differently about it if I had come up with
the idea. Actually, I am really impressed with the discoveries that arose from my original patch,
including recognizing that the min3/max3 generated worse code than nested macro calls. I had
never considered that possibility.

> How useful is W=2 anyway?  Has anyone found
> a bug using it?

Yes. But it is really hard to find anything useful when my normal kernel build throws 125000
warnings - for completely expected things - in W=2 builds. We do routinely build our drivers
with W=12, but we have a special hack to ignore the large number of warnings that the kernel
include files generate, while still getting warnings from our driver's code and include files.
They do sometimes catch problems.

> The number of warnings in default builds is already way
> too high :(

Do you mean the number of warnings enabled, or the number of warning messages being generated?
I am a fan of enabling lots of warnings, but it is not effective if doing so emits thousands of
messages. Back in the 2.4 kernel era, there was a time when I maintained a MIPS-based kernel
that built completely cleanly with way more warnings enabled than W=12 uses today. It took work
to get to that state, but we were able to maintain that for several kernel versions for our particular environment. These days, the kernel as a whole is in a much better state than we
started with for that old 2.4 kernel.

I'll say that there aren't really that many warnings that are the result of nested min/max
macros. There are *much* heavier hitters out there. For example "nested externs" account for
tens of thousands of warnings in W=2 builds. Such noise makes using W=2 kind of ridiculous
on the whole kernel. I'd like it to not be so ridiculous eventually. Some 60 patches got my
build down under 1400 W=2 warnings. Then I realized that I had pushed that rock too far up the
hill without getting feedback on what I was doing.

I have been working on patches that add macros to allow disabling certain warnings in select
pieces of code. In that way the warning is silenced, but the macro remains as a sign to the
reader that there is something special going on. Unfortunately, those macros fail miserably
in the expansion of any macro called in the parameter of other macros, as the compile time
asserts sometimes are. And the compile time asserts make use of a nested extern by design.

It is enough to make me want to suggest dropping the nested externs warning from W=2, but
recognize that every nested extern that actually calls a function is a redundant prototype
declaration that could get to be inconsistent with the function it calls. They have disentangled
the include files some, but added a bit of risk and maintenance burden.

This whole area is full of judgement calls, so people will disagree. Hopefully any debate will
be worthwhile. My first reaction to the patch above was that it was kind of wild, but it does
well and truly fix the problem and does no harm on compilers that don't have __COUNTER__, so
I am ok with it. Yes, I have compiled it.

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]

Michal,

On Sep 12, 2014, at 4:37 PM, Michal Nazarewicz <mina86@mina86.com> wrote:

> On Fri, Sep 12 2014, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Thu, 11 Sep 2014 23:39:36 +0200 Michal Nazarewicz <mina86@mina86.com> wrote:
>> 
>>> Because min and max macros use the same variable names no matter
>>> how many times they are called (or how deep the nesting of their
>>> calls), each time min or max calls are nested, the same variables
>>> are declared.  This is especially noisy after min3 and max3 have
>>> been changed to nest min/max calls.
>>> 
>>> Using __COUNTER__ solves the problem since each variable will get
>>> a unique number aadded to it.  The code will still work even if
>>> the compiler does not support __COUNTER__, but then the protection
>>> from shadow warning won't work.
>>> 
>>> The same applies to min_t and max_t macros.
>>> 
>>> ...
>>> 
>>> --- a/include/linux/kernel.h
>>> +++ b/include/linux/kernel.h
>>> @@ -695,15 +695,27 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>>> #endif /* CONFIG_TRACING */
>>> 
>>> /*
>>> + * Preprocessor magic generating unique identifiers to avoid -Wshadow warnings
>>> + * used by min, max, min_t and max_t macros.  cnt is __COUNTER__, op is the
>>> + * comparison operator; tx (ty) is type of the first (second) argument,
>>> + * xx (yy) is name of a temporary variable to hold the first (second) argument,
>>> + * and x (y) is the first (second) argument.
>>> + */
>>> +#define _min_max_var(cnt, base) _mm_ ## cnt ## base
>>> +#define _min_max__(op, tx, xx, x, ty, yy, y) ({		\
>>> +	tx xx = (x);					\
>>> +	ty yy = (y);					\
>>> +	(void) (&xx == &yy);				\
>>> +	xx op yy ? xx : yy; })
>>> +#define _min_max_(cnt, op, tx, x, ty, y)		\
>>> +	_min_max__(op, tx, _min_max_var(cnt, a), x, ty, _min_max_var(cnt, b), y)
>>> +#define _min_max(...) _min_max_(__COUNTER__, __VA_ARGS__)
>> 
>> The fact that __COUNTER__ is used in compiler-gcc4.h but not in
>> compiler-gcc3.h makes me suspicious about its availability?
> 
> http://gcc.gnu.org/ml/gcc-patches/2007-05/msg01579.html so looks like it
> has 7 years.  But as the commit message says, the code will still work,
> even w/o working __COUNTER__.
> 
>> I do think that [1/2] made the code significantly worse-looking
> 
> Oh?  I actually thought [1/2] makes it nicer by having a single place
> where the min/max logic is implemented.

It does have that going for it.

>> and this one is getting crazy.  How useful is W=2 anyway?
> 
> I actually do agree with that.  I didn't have high hopes about getting
> this patch accepted, but wanted to send it out to show that it can be
> done, if it's really deemed useful.

Well, I learned something from it. Thank you for teaching this old dog a new trick.

>> Has anyone found a bug using it?  The number of warnings in default
>> builds is already way too high :(

-- 
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH 2/2] kernel.h: use __COUNTER__ in min and max macros to avoid -Wshadow warnings

[Andrew Morton]

On Fri, 12 Sep 2014 23:43:51 +0000 "Rustad, Mark D" <mark.d.rustad@intel.com> wrote:

> Do you mean the number of warnings enabled, or the number of warning messages being generated?

The latter.

My problem is I use crufty old compilers so a number of the warnings I
see aren't seen by sane people and it' snot worth mucking up the source
to fix them.

otoh crufty new compilers add warnings which I won't see...

[PATCH] include: kernel.h: deduplicate code implementing clamp* macros

[Michal Nazarewicz]

Instead of open-coding clamp_t macro min_t and max_t the way clamp
macro does and instead of open-coding clamp_val simply use clamp_t.
Furthermore, normalise argument naming in the macros to be lo and
hi.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/linux/kernel.h | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

This is an exceprt from the other bigger patch which had some
controversy I assume because of the way it reimplemented min and max.
This only contains refactoring of clamp which I believe should be
uncontroversial.

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index c39c69d..40728cf 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -731,7 +731,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @lo: lowest allowable value
  * @hi: highest allowable value
  *
- * This macro does strict typechecking of min/max to make sure they are of the
+ * This macro does strict typechecking of lo/hi to make sure they are of the
  * same type as val.  See the unnecessary pointer comparisons.
  */
 #define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)
@@ -756,36 +756,26 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * clamp_t - return a value clamped to a given range using a given type
  * @type: the type of variable to use
  * @val: current value
- * @min: minimum allowable value
- * @max: maximum allowable value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
  *
  * This macro does no typechecking and uses temporary variables of type
  * 'type' to make all the comparisons.
  */
-#define clamp_t(type, val, min, max) ({		\
-	type __val = (val);			\
-	type __min = (min);			\
-	type __max = (max);			\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
+#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
 
 /**
  * clamp_val - return a value clamped to a given range using val's type
  * @val: current value
- * @min: minimum allowable value
- * @max: maximum allowable value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
  *
  * This macro does no typechecking and uses temporary variables of whatever
  * type the input argument 'val' is.  This is useful when val is an unsigned
  * type and min and max are literals that will otherwise be assigned a signed
  * integer type.
  */
-#define clamp_val(val, min, max) ({		\
-	typeof(val) __val = (val);		\
-	typeof(val) __min = (min);		\
-	typeof(val) __max = (max);		\
-	__val = __val < __min ? __min: __val;	\
-	__val > __max ? __max: __val; })
+#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
 
 
 /*
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH] include: kernel.h: deduplicate code implementing clamp* macros

[Hagen Paul Pfeifer]

On 25 September 2014 17:50, Michal Nazarewicz <mina86@mina86.com> wrote:

> - * @min: minimum allowable value
> - * @max: maximum allowable value
> + * @lo: minimum allowable value
> + * @hi: maximum allowable value

Ok, but why do you rename min/max to low/high after so many years?
min/max is just perfect

Hagen

Re: [PATCH] include: kernel.h: deduplicate code implementing clamp* macros

[Michal Nazarewicz]

On Thu, Sep 25 2014, Hagen Paul Pfeifer <hagen@jauu.net> wrote:
> On 25 September 2014 17:50, Michal Nazarewicz <mina86@mina86.com> wrote:
>
>> - * @min: minimum allowable value
>> - * @max: maximum allowable value
>> + * @lo: minimum allowable value
>> + * @hi: maximum allowable value
>
> Ok, but why do you rename min/max to low/high after so many years?
> min/max is just perfect

To match argument names of the clamp macro (which got renamed when
rewriting min3, max3 and clamp), but I don't really care either way.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--