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
| ||
|
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