| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.LFD.2.20.1511122112410.5826@knanqh.ubzr>
Date: Thu, 12 Nov 2015 21:23:09 -0500 (EST)
From: Nicolas Pitre <nicolas.pitre@...aro.org>
To: Michal Nazarewicz <mina86@...a86.com>
cc: Andrew Morton <akpm@...ux-foundation.org>,
John Stultz <john.stultz@...aro.org>,
LKML <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>, Tejun Heo <tj@...nel.org>,
Jeff Epler <jepler@...ythonic.net>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Wey-Yi Guy <wey-yi.w.guy@...el.com>
Subject: Re: [PATCH] include: kernel.h: change abs macro so it uses consistent
return type
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@...a86.com>
> Cc: Nicolas Pitre <nicolas.pitre@...aro.org>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> Cc: Wey-Yi Guy <wey-yi.w.guy@...el.com>
Reviewwed-by: Nicolas Pitre <nico@...aro.org>
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
>
>
Powered by blists - more mailing lists