Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1268548 > unrolled thread
| Started by | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| First post | 2015-11-13 03:10 +0100 |
| Last post | 2015-11-13 03:30 +0100 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] include: kernel.h: change abs macro so it uses consistent return type Michal Nazarewicz <mina86@mina86.com> - 2015-11-13 03:10 +0100
Re: [PATCH] include: kernel.h: change abs macro so it uses consistent return type Nicolas Pitre <nicolas.pitre@linaro.org> - 2015-11-13 03:30 +0100
| From | Michal Nazarewicz <mina86@mina86.com> |
|---|---|
| Date | 2015-11-13 03:10 +0100 |
| Subject | [PATCH] include: kernel.h: change abs macro so it uses consistent return type |
| Message-ID | <qubYn-gb-13@gated-at.bofh.it> |
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@mina86.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Wey-Yi Guy <wey-yi.w.guy@intel.com>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Nicolas Pitre <nicolas.pitre@linaro.org> |
|---|---|
| Date | 2015-11-13 03:30 +0100 |
| Subject | Re: [PATCH] include: kernel.h: change abs macro so it uses consistent return type |
| Message-ID | <quchH-nf-1@gated-at.bofh.it> |
| In reply to | #1268548 |
[Multipart message — attachments visible in raw view] — view raw
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@mina86.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Wey-Yi Guy <wey-yi.w.guy@intel.com>
Reviewwed-by: Nicolas Pitre <nico@linaro.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
>
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web