| Message ID | 1442279124-7309-6-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
* 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/
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/
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/
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; \
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(+)