[RFC,5/5] abs(): Provide build error on passing 64bit value to abs()

Message ID 1442279124-7309-6-git-send-email-john.stultz@linaro.org
State New
Headers show

Commit Message

John Stultz Sept. 15, 2015, 1:05 a.m.
As noted in the comment above abs():
 "abs() should not be used for 64-bit types (s64, u64, long long)
  - use abs64()  for those."

Unfortunately, its quite easy to pass 64-bit values to abs()
accidentally, and the compiler provides no warning when the
returned value is erroniously capped at 32-bits.

So this patch tries to make it easier to detect when 64-bit
values are passed to abs() by generating a build error.

Obviously, since this causes build errors, this patch is last
in the series, and I tried to fix up all of the issues I ran
into in my build testing. But there are likely still some out
there.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/kernel.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Ingo Molnar Sept. 15, 2015, 5:22 a.m. | #1
* John Stultz <john.stultz@linaro.org> wrote:

> As noted in the comment above abs():
>  "abs() should not be used for 64-bit types (s64, u64, long long)
>   - use abs64()  for those."
> 
> Unfortunately, its quite easy to pass 64-bit values to abs()
> accidentally, and the compiler provides no warning when the
> returned value is erroniously capped at 32-bits.
> 
> So this patch tries to make it easier to detect when 64-bit
> values are passed to abs() by generating a build error.
> 
> Obviously, since this causes build errors, this patch is last
> in the series, and I tried to fix up all of the issues I ran
> into in my build testing. But there are likely still some out
> there.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  include/linux/kernel.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 5582410..6f01151 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -208,6 +208,9 @@ extern int _cond_resched(void);
>   */
>  #define abs(x) ({						\
>  		long ret;					\
> +		compiletime_assert(				\
> +			!(sizeof(typeof(x)) > sizeof(long)),	\
> +			"abs() should not be used for 64-bit types - use abs64()");\
>  		if (sizeof(x) == sizeof(long)) {		\
>  			long __x = (x);				\
>  			ret = (__x < 0) ? -__x : __x;		\

I think this should be a compiletime_warning() - that will be visible enough.

Thanks,

	Ingo
--
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/
Linus Torvalds Sept. 15, 2015, 11:52 p.m. | #2
On Mon, Sep 14, 2015 at 10:22 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> I think this should be a compiletime_warning() - that will be visible enough.

So the problem with this is that by now most kernel developers are on 64-bit.

And that "sizeof(typeof(x)) > sizeof(long))" would effectively never
trigger on 64-bit architectures, so almost no core developers would
see it. Yes, it would be caught by buildbots etc, but that's really
not very convenient. The new errors would be noticed too late, because
the actual *developers* wouldn't see them.

(Not to mention that the "typeof()" in that expression is redundant ;)

So I think the "auto-expand to 's64' using __builtin_choose_expr()" is
the preferable model, and get rid of abs64() entirely. It has very few
uses.

                  Linus
--
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/
John Stultz Sept. 18, 2015, 3:12 a.m. | #3
On Wed, Sep 16, 2015 at 5:57 AM, Michal Nazarewicz <mina86@mina86.com> wrote:
> For 64-bit arguments, abs macro casts it to an int which leads to lost
> precision and may cause incorrect results.  To deal with 64-bit types
> abs64 macro has been introduced but still there are places where abs
> macro is used incorrectly.
>
> To deal with the problem, expand abs macro such that it operates on s64
> type when dealing with 64-bit types while still returning long when
> dealing with smaller types.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

Sorry this is a little late, but I did run this patch through my tests
on a 32bit system (against 4.2, since my localized fix has already
landed in Linus' head) to make sure it resolved the bug I saw and it
did.

So,
Tested-by: John Stultz <john.stultz@linaro.org>

thanks!
-john
--
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/

Patch hide | download patch | download mbox

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5582410..6f01151 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -208,6 +208,9 @@  extern int _cond_resched(void);
  */
 #define abs(x) ({						\
 		long ret;					\
+		compiletime_assert(				\
+			!(sizeof(typeof(x)) > sizeof(long)),	\
+			"abs() should not be used for 64-bit types - use abs64()");\
 		if (sizeof(x) == sizeof(long)) {		\
 			long __x = (x);				\
 			ret = (__x < 0) ? -__x : __x;		\