Re: [PATCH] include: kernel.h: change abs macro so it uses consistent return type

[Nicolas Pitre]

On Fri, 13 Nov 2015, Michal Nazarewicz wrote:

> Rewrite the abs macro such that it return type does not depend on the
> architecture and no unexpected type conversion happen inside of it.  The
> only conversion is from unsigned to signed type.  char is left as
> a return type but treated as a signed type regradless of itâs actual
> signedness.
> 
> With the old version, int arguments were promoted to long and depending
> on architecture a long argument might result in s64 or long return type
> (which may or may not be the same).
> 
> Signed-off-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> Cc: Nicolas Pitre <nicolas.pitre@xxxxxxxxxx>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Cc: Wey-Yi Guy <wey-yi.w.guy@xxxxxxxxx>

Reviewwed-by: Nicolas Pitre <nico@xxxxxxxxxx>

It might be worth mentioning in the changelog for those who might wonder:

__builtin_types_compatible_p(unsigned char, char) is always false, and
__builtin_types_compatible_p(signed char, char) is also always false.

That's the reason for the unqualified char special case.

> ---
>  drivers/iio/industrialio-core.c          |  9 ++++----
>  drivers/net/wireless/iwlwifi/dvm/calib.c |  2 +-
>  include/linux/kernel.h                   | 36 ++++++++++++++++----------------
>  3 files changed, 23 insertions(+), 24 deletions(-)
> 
>  This came after some back and forth with Nicolas.  The current macro
>  has different return type (for the same input type) depending on
>  architecture which might be midly iritating.
> 
>  An alternative version would promote to int like so:
> 
> 	#define abs(x)	__abs_choose_expr(x, long long,			\
> 			__abs_choose_expr(x, long,			\
> 			__builtin_choose_expr(				\
> 				sizeof(x) <= sizeof(int),		\
> 				({ int __x = (x); __x<0?-__x:__x; }),	\
> 				((void)0))))
> 
>  I have no preference but imagine Linus might. :] Nicolas argument
>  against is that promoting to int causes iconsistent behaviour:
> 
> 	int main(void) {
> 		unsigned short a = 0, b = 1, c = a - b;
> 		unsigned short d = abs(a - b);
> 		unsigned short e = abs(c);
> 		printf("%u %u\n", d, e);  // prints: 1 65535
> 	}
> 
>  Then again, no sane person expect consistent behaviour from C integer
>  arithmetic. ;)
> 
>  Compile tested with âmake allmodconfig && make bzImage modulesâ on x86_64.
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 208358f..37697d5 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -433,16 +433,15 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
>  		scale_db = true;
>  	case IIO_VAL_INT_PLUS_MICRO:
>  		if (vals[1] < 0)
> -			return sprintf(buf, "-%ld.%06u%s\n", abs(vals[0]),
> -					-vals[1],
> -				scale_db ? " dB" : "");
> +			return sprintf(buf, "-%d.%06u%s\n", abs(vals[0]),
> +				       -vals[1], scale_db ? " dB" : "");
>  		else
>  			return sprintf(buf, "%d.%06u%s\n", vals[0], vals[1],
>  				scale_db ? " dB" : "");
>  	case IIO_VAL_INT_PLUS_NANO:
>  		if (vals[1] < 0)
> -			return sprintf(buf, "-%ld.%09u\n", abs(vals[0]),
> -					-vals[1]);
> +			return sprintf(buf, "-%d.%09u\n", abs(vals[0]),
> +				       -vals[1]);
>  		else
>  			return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>  	case IIO_VAL_FRACTIONAL:
> diff --git a/drivers/net/wireless/iwlwifi/dvm/calib.c b/drivers/net/wireless/iwlwifi/dvm/calib.c
> index 20e6aa9..c148085 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/calib.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/calib.c
> @@ -901,7 +901,7 @@ static void iwlagn_gain_computation(struct iwl_priv *priv,
>  		/* bound gain by 2 bits value max, 3rd bit is sign */
>  		data->delta_gain_code[i] =
>  			min(abs(delta_g),
> -			(long) CHAIN_NOISE_MAX_DELTA_GAIN_CODE);
> +			(s32) CHAIN_NOISE_MAX_DELTA_GAIN_CODE);
>  
>  		if (delta_g < 0)
>  			/*
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 350dfb0..59c8c2a 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -202,26 +202,26 @@ extern int _cond_resched(void);
>  
>  /**
>   * abs - return absolute value of an argument
> - * @x: the value.  If it is unsigned type, it is converted to signed type first
> - *   (s64, long or int depending on its size).
> + * @x: the value.  If it is unsigned type, it is converted to signed type first.
> + *     char is treated as if it was signed (regardless of whether it really is)
> + *     but macroâs return type is preserved as char.
>   *
> - * Return: an absolute value of x.  If x is 64-bit, macro's return type is s64,
> - *   otherwise it is signed long.
> + * Return: an absolute value of x.
>   */
> -#define abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({	\
> -		s64 __x = (x);						\
> -		(__x < 0) ? -__x : __x;					\
> -	}), ({								\
> -		long ret;						\
> -		if (sizeof(x) == sizeof(long)) {			\
> -			long __x = (x);					\
> -			ret = (__x < 0) ? -__x : __x;			\
> -		} else {						\
> -			int __x = (x);					\
> -			ret = (__x < 0) ? -__x : __x;			\
> -		}							\
> -		ret;							\
> -	}))
> +#define abs(x)	__abs_choose_expr(x, long long,				\
> +		__abs_choose_expr(x, long,				\
> +		__abs_choose_expr(x, int,				\
> +		__abs_choose_expr(x, short,				\
> +		__abs_choose_expr(x, char,				\
> +		__builtin_choose_expr(					\
> +			__builtin_types_compatible_p(typeof(x), char),	\
> +			(char)({ signed char __x = (x); __x<0?-__x:__x; }), \
> +			((void)0)))))))
> +
> +#define __abs_choose_expr(x, type, other) __builtin_choose_expr(	\
> +	__builtin_types_compatible_p(typeof(x),   signed type) ||	\
> +	__builtin_types_compatible_p(typeof(x), unsigned type),		\
> +	({ signed type __x = (x); __x < 0 ? -__x : __x; }), other)
>  
>  /**
>   * reciprocal_scale - "scale" a value into range [0, ep_ro)
> -- 
> 2.6.0.rc2.230.g3dd15c0
> 
>