2021-05-11 20:41:36

by Rikard Falkeborn

[permalink] [raw]
Subject: [PATCH] linux/bits.h: Fix compilation error with GENMASK

GENMASK() has an input check which uses __builtin_choose_expr() to enable
a compile time sanity check of its inputs if they are known at compile
time. However, it turns out that __builtin_constant_p() does not always
return a compile time constant [0]. It was thought this problem was fixed
with gcc 4.9 [1], but apparently this is not the case [2].

Switch to use __is_constexpr() instead which always returns a compile
time constant, regardless of its inputs.

[0]: https://lore.kernel.org/lkml/[email protected]
[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
[2]: https://lore.kernel.org/lkml/[email protected]

Reported-by: Tetsuo Handa <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
Feedback on placing __is_constexpr() in const.h is welcome, at least the
name is appropriate...

include/linux/bits.h | 2 +-
include/linux/const.h | 8 ++++++++
include/linux/minmax.h | 10 ++--------
tools/include/linux/bits.h | 2 +-
tools/include/linux/const.h | 8 ++++++++
5 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7f475d59a097..87d112650dfb 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -22,7 +22,7 @@
#include <linux/build_bug.h>
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
- __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+ __is_constexpr((l) > (h)), (l) > (h), 0)))
#else
/*
* BUILD_BUG_ON_ZERO is not available in h files included from asm files,
diff --git a/include/linux/const.h b/include/linux/const.h
index 81b8aae5a855..435ddd72d2c4 100644
--- a/include/linux/const.h
+++ b/include/linux/const.h
@@ -3,4 +3,12 @@

#include <vdso/const.h>

+/*
+ * This returns a constant expression while determining if an argument is
+ * a constant expression, most importantly without evaluating the argument.
+ * Glory to Martin Uecker <[email protected]>
+ */
+#define __is_constexpr(x) \
+ (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
+
#endif /* _LINUX_CONST_H */
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index c0f57b0c64d9..5433c08fcc68 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -2,6 +2,8 @@
#ifndef _LINUX_MINMAX_H
#define _LINUX_MINMAX_H

+#include <linux/const.h>
+
/*
* min()/max()/clamp() macros must accomplish three things:
*
@@ -17,14 +19,6 @@
#define __typecheck(x, y) \
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))

-/*
- * This returns a constant expression while determining if an argument is
- * a constant expression, most importantly without evaluating the argument.
- * Glory to Martin Uecker <[email protected]>
- */
-#define __is_constexpr(x) \
- (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
-
#define __no_side_effects(x, y) \
(__is_constexpr(x) && __is_constexpr(y))

diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
index 7f475d59a097..87d112650dfb 100644
--- a/tools/include/linux/bits.h
+++ b/tools/include/linux/bits.h
@@ -22,7 +22,7 @@
#include <linux/build_bug.h>
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
- __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+ __is_constexpr((l) > (h)), (l) > (h), 0)))
#else
/*
* BUILD_BUG_ON_ZERO is not available in h files included from asm files,
diff --git a/tools/include/linux/const.h b/tools/include/linux/const.h
index 81b8aae5a855..435ddd72d2c4 100644
--- a/tools/include/linux/const.h
+++ b/tools/include/linux/const.h
@@ -3,4 +3,12 @@

#include <vdso/const.h>

+/*
+ * This returns a constant expression while determining if an argument is
+ * a constant expression, most importantly without evaluating the argument.
+ * Glory to Martin Uecker <[email protected]>
+ */
+#define __is_constexpr(x) \
+ (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
+
#endif /* _LINUX_CONST_H */
--
2.31.1


2021-05-12 13:19:06

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

(+ Arnd)

On Tue, 11 May 2021 at 22:37, Rikard Falkeborn
<[email protected]> wrote:
>
> GENMASK() has an input check which uses __builtin_choose_expr() to enable
> a compile time sanity check of its inputs if they are known at compile
> time. However, it turns out that __builtin_constant_p() does not always
> return a compile time constant [0]. It was thought this problem was fixed
> with gcc 4.9 [1], but apparently this is not the case [2].
>
> Switch to use __is_constexpr() instead which always returns a compile
> time constant, regardless of its inputs.
>
> [0]: https://lore.kernel.org/lkml/[email protected]
> [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> [2]: https://lore.kernel.org/lkml/[email protected]
>
> Reported-by: Tetsuo Handa <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Feedback on placing __is_constexpr() in const.h is welcome, at least the
> name is appropriate...
>
> include/linux/bits.h | 2 +-
> include/linux/const.h | 8 ++++++++
> include/linux/minmax.h | 10 ++--------
> tools/include/linux/bits.h | 2 +-
> tools/include/linux/const.h | 8 ++++++++
> 5 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 7f475d59a097..87d112650dfb 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -22,7 +22,7 @@
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> + __is_constexpr((l) > (h)), (l) > (h), 0)))
> #else
> /*
> * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> diff --git a/include/linux/const.h b/include/linux/const.h
> index 81b8aae5a855..435ddd72d2c4 100644
> --- a/include/linux/const.h
> +++ b/include/linux/const.h
> @@ -3,4 +3,12 @@
>
> #include <vdso/const.h>
>
> +/*
> + * This returns a constant expression while determining if an argument is
> + * a constant expression, most importantly without evaluating the argument.
> + * Glory to Martin Uecker <[email protected]>
> + */
> +#define __is_constexpr(x) \
> + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> +
> #endif /* _LINUX_CONST_H */
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index c0f57b0c64d9..5433c08fcc68 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -2,6 +2,8 @@
> #ifndef _LINUX_MINMAX_H
> #define _LINUX_MINMAX_H
>
> +#include <linux/const.h>
> +
> /*
> * min()/max()/clamp() macros must accomplish three things:
> *
> @@ -17,14 +19,6 @@
> #define __typecheck(x, y) \
> (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>
> -/*
> - * This returns a constant expression while determining if an argument is
> - * a constant expression, most importantly without evaluating the argument.
> - * Glory to Martin Uecker <[email protected]>
> - */
> -#define __is_constexpr(x) \
> - (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> -
> #define __no_side_effects(x, y) \
> (__is_constexpr(x) && __is_constexpr(y))
>
> diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> index 7f475d59a097..87d112650dfb 100644
> --- a/tools/include/linux/bits.h
> +++ b/tools/include/linux/bits.h
> @@ -22,7 +22,7 @@
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> + __is_constexpr((l) > (h)), (l) > (h), 0)))
> #else
> /*
> * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> diff --git a/tools/include/linux/const.h b/tools/include/linux/const.h
> index 81b8aae5a855..435ddd72d2c4 100644
> --- a/tools/include/linux/const.h
> +++ b/tools/include/linux/const.h
> @@ -3,4 +3,12 @@
>
> #include <vdso/const.h>
>
> +/*
> + * This returns a constant expression while determining if an argument is
> + * a constant expression, most importantly without evaluating the argument.
> + * Glory to Martin Uecker <[email protected]>
> + */
> +#define __is_constexpr(x) \
> + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> +
> #endif /* _LINUX_CONST_H */
> --
> 2.31.1
>

2021-05-21 06:51:25

by Rikard Falkeborn

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

On Wed, May 12, 2021 at 02:53:27PM +0200, Ard Biesheuvel wrote:
> (+ Arnd)
>
> On Tue, 11 May 2021 at 22:37, Rikard Falkeborn
> <[email protected]> wrote:
> >
> > GENMASK() has an input check which uses __builtin_choose_expr() to enable
> > a compile time sanity check of its inputs if they are known at compile
> > time. However, it turns out that __builtin_constant_p() does not always
> > return a compile time constant [0]. It was thought this problem was fixed
> > with gcc 4.9 [1], but apparently this is not the case [2].
> >
> > Switch to use __is_constexpr() instead which always returns a compile
> > time constant, regardless of its inputs.
> >
> > [0]: https://lore.kernel.org/lkml/[email protected]
> > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> > [2]: https://lore.kernel.org/lkml/[email protected]
> >
> > Reported-by: Tetsuo Handa <[email protected]>
> > Signed-off-by: Rikard Falkeborn <[email protected]>
> > ---
> > Feedback on placing __is_constexpr() in const.h is welcome, at least the
> > name is appropriate...
> >
> > include/linux/bits.h | 2 +-
> > include/linux/const.h | 8 ++++++++
> > include/linux/minmax.h | 10 ++--------
> > tools/include/linux/bits.h | 2 +-
> > tools/include/linux/const.h | 8 ++++++++
> > 5 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 7f475d59a097..87d112650dfb 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -22,7 +22,7 @@
> > #include <linux/build_bug.h>
> > #define GENMASK_INPUT_CHECK(h, l) \
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > + __is_constexpr((l) > (h)), (l) > (h), 0)))
> > #else
> > /*
> > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > diff --git a/include/linux/const.h b/include/linux/const.h
> > index 81b8aae5a855..435ddd72d2c4 100644
> > --- a/include/linux/const.h
> > +++ b/include/linux/const.h
> > @@ -3,4 +3,12 @@
> >
> > #include <vdso/const.h>
> >
> > +/*
> > + * This returns a constant expression while determining if an argument is
> > + * a constant expression, most importantly without evaluating the argument.
> > + * Glory to Martin Uecker <[email protected]>
> > + */
> > +#define __is_constexpr(x) \
> > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > +
> > #endif /* _LINUX_CONST_H */
> > diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> > index c0f57b0c64d9..5433c08fcc68 100644
> > --- a/include/linux/minmax.h
> > +++ b/include/linux/minmax.h
> > @@ -2,6 +2,8 @@
> > #ifndef _LINUX_MINMAX_H
> > #define _LINUX_MINMAX_H
> >
> > +#include <linux/const.h>
> > +
> > /*
> > * min()/max()/clamp() macros must accomplish three things:
> > *
> > @@ -17,14 +19,6 @@
> > #define __typecheck(x, y) \
> > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> >
> > -/*
> > - * This returns a constant expression while determining if an argument is
> > - * a constant expression, most importantly without evaluating the argument.
> > - * Glory to Martin Uecker <[email protected]>
> > - */
> > -#define __is_constexpr(x) \
> > - (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > -
> > #define __no_side_effects(x, y) \
> > (__is_constexpr(x) && __is_constexpr(y))
> >
> > diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> > index 7f475d59a097..87d112650dfb 100644
> > --- a/tools/include/linux/bits.h
> > +++ b/tools/include/linux/bits.h
> > @@ -22,7 +22,7 @@
> > #include <linux/build_bug.h>
> > #define GENMASK_INPUT_CHECK(h, l) \
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > + __is_constexpr((l) > (h)), (l) > (h), 0)))
> > #else
> > /*
> > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > diff --git a/tools/include/linux/const.h b/tools/include/linux/const.h
> > index 81b8aae5a855..435ddd72d2c4 100644
> > --- a/tools/include/linux/const.h
> > +++ b/tools/include/linux/const.h
> > @@ -3,4 +3,12 @@
> >
> > #include <vdso/const.h>
> >
> > +/*
> > + * This returns a constant expression while determining if an argument is
> > + * a constant expression, most importantly without evaluating the argument.
> > + * Glory to Martin Uecker <[email protected]>
> > + */
> > +#define __is_constexpr(x) \
> > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > +
> > #endif /* _LINUX_CONST_H */
> > --
> > 2.31.1
> >

Friendly ping.

Rikard

2021-05-21 07:25:31

by Yury Norov

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

On Thu, May 20, 2021 at 08:46:40PM +0200, Rikard Falkeborn wrote:
> On Wed, May 12, 2021 at 02:53:27PM +0200, Ard Biesheuvel wrote:
> > (+ Arnd)
> >
> > On Tue, 11 May 2021 at 22:37, Rikard Falkeborn
> > <[email protected]> wrote:
> > >
> > > GENMASK() has an input check which uses __builtin_choose_expr() to enable
> > > a compile time sanity check of its inputs if they are known at compile
> > > time. However, it turns out that __builtin_constant_p() does not always
> > > return a compile time constant [0]. It was thought this problem was fixed
> > > with gcc 4.9 [1], but apparently this is not the case [2].
> > >
> > > Switch to use __is_constexpr() instead which always returns a compile
> > > time constant, regardless of its inputs.
> > >
> > > [0]: https://lore.kernel.org/lkml/[email protected]
> > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> > > [2]: https://lore.kernel.org/lkml/[email protected]
> > >
> > > Reported-by: Tetsuo Handa <[email protected]>
> > > Signed-off-by: Rikard Falkeborn <[email protected]>
> > > ---
> > > Feedback on placing __is_constexpr() in const.h is welcome, at least the
> > > name is appropriate...
> > >
> > > include/linux/bits.h | 2 +-
> > > include/linux/const.h | 8 ++++++++
> > > include/linux/minmax.h | 10 ++--------
> > > tools/include/linux/bits.h | 2 +-
> > > tools/include/linux/const.h | 8 ++++++++
> > > 5 files changed, 20 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > > index 7f475d59a097..87d112650dfb 100644
> > > --- a/include/linux/bits.h
> > > +++ b/include/linux/bits.h
> > > @@ -22,7 +22,7 @@
> > > #include <linux/build_bug.h>
> > > #define GENMASK_INPUT_CHECK(h, l) \
> > > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > + __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > #else
> > > /*
> > > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > > diff --git a/include/linux/const.h b/include/linux/const.h
> > > index 81b8aae5a855..435ddd72d2c4 100644
> > > --- a/include/linux/const.h
> > > +++ b/include/linux/const.h
> > > @@ -3,4 +3,12 @@
> > >
> > > #include <vdso/const.h>
> > >
> > > +/*
> > > + * This returns a constant expression while determining if an argument is
> > > + * a constant expression, most importantly without evaluating the argument.
> > > + * Glory to Martin Uecker <[email protected]>
> > > + */
> > > +#define __is_constexpr(x) \
> > > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > > +
> > > #endif /* _LINUX_CONST_H */
> > > diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> > > index c0f57b0c64d9..5433c08fcc68 100644
> > > --- a/include/linux/minmax.h
> > > +++ b/include/linux/minmax.h
> > > @@ -2,6 +2,8 @@
> > > #ifndef _LINUX_MINMAX_H
> > > #define _LINUX_MINMAX_H
> > >
> > > +#include <linux/const.h>
> > > +
> > > /*
> > > * min()/max()/clamp() macros must accomplish three things:
> > > *
> > > @@ -17,14 +19,6 @@
> > > #define __typecheck(x, y) \
> > > (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> > >
> > > -/*
> > > - * This returns a constant expression while determining if an argument is
> > > - * a constant expression, most importantly without evaluating the argument.
> > > - * Glory to Martin Uecker <[email protected]>
> > > - */
> > > -#define __is_constexpr(x) \
> > > - (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > > -
> > > #define __no_side_effects(x, y) \
> > > (__is_constexpr(x) && __is_constexpr(y))
> > >
> > > diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> > > index 7f475d59a097..87d112650dfb 100644
> > > --- a/tools/include/linux/bits.h
> > > +++ b/tools/include/linux/bits.h
> > > @@ -22,7 +22,7 @@
> > > #include <linux/build_bug.h>
> > > #define GENMASK_INPUT_CHECK(h, l) \
> > > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > + __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > #else
> > > /*
> > > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > > diff --git a/tools/include/linux/const.h b/tools/include/linux/const.h
> > > index 81b8aae5a855..435ddd72d2c4 100644
> > > --- a/tools/include/linux/const.h
> > > +++ b/tools/include/linux/const.h
> > > @@ -3,4 +3,12 @@
> > >
> > > #include <vdso/const.h>
> > >
> > > +/*
> > > + * This returns a constant expression while determining if an argument is
> > > + * a constant expression, most importantly without evaluating the argument.
> > > + * Glory to Martin Uecker <[email protected]>
> > > + */
> > > +#define __is_constexpr(x) \
> > > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > > +
> > > #endif /* _LINUX_CONST_H */
> > > --
> > > 2.31.1
> > >
>
> Friendly ping.

I added this patch in my Bitmap tree, but since it's actually a build
bug fix, I think it's worth to move it faster with Arnd's or Andrew's
trees?

2021-05-21 12:46:04

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

On Thu, May 20, 2021 at 01:41:12PM -0700, Andrew Morton wrote:
> On Tue, 11 May 2021 22:37:15 +0200 Rikard Falkeborn <[email protected]> wrote:
>
> > --- a/include/linux/const.h
> > +++ b/include/linux/const.h
> > @@ -3,4 +3,12 @@
> >
> > #include <vdso/const.h>
> >
> > +/*
> > + * This returns a constant expression while determining if an argument is
> > + * a constant expression, most importantly without evaluating the argument.
> > + * Glory to Martin Uecker <[email protected]>
> > + */
> > +#define __is_constexpr(x) \
> > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
>
> Boggle.
>
> Could someone please sometime enhance that comment a bit? What need
> does this thing satisfy and how on earth does it work?

Some summary based on (links from) https://vegard.wiki/w/is_constexpr() ?

--
With Best Regards,
Andy Shevchenko


2021-05-21 19:38:26

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

On Thu, May 20, 2021 at 9:44 PM Yury Norov <[email protected]> wrote:
> On Thu, May 20, 2021 at 08:46:40PM +0200, Rikard Falkeborn wrote:
> > > > +/*
> > > > + * This returns a constant expression while determining if an argument is
> > > > + * a constant expression, most importantly without evaluating the argument.
> > > > + * Glory to Martin Uecker <[email protected]>
> > > > + */
> > > > +#define __is_constexpr(x) \
> > > > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > > > +
> > > > #endif /* _LINUX_CONST_H */
> > > > --
> > > > 2.31.1
> > > >
> >
> > Friendly ping.
>
> I added this patch in my Bitmap tree, but since it's actually a build
> bug fix, I think it's worth to move it faster with Arnd's or Andrew's
> trees?

I have a few days off next week and won't be able to validate it before then,
so it would be faster to have Andrew pick it up.

Moving the definition to const.h seem reasonable to me here, feel
free to add

Acked-by: Arnd Bergmann <[email protected]>

2021-05-21 19:41:53

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

On Tue, 11 May 2021 22:37:15 +0200 Rikard Falkeborn <[email protected]> wrote:

> --- a/include/linux/const.h
> +++ b/include/linux/const.h
> @@ -3,4 +3,12 @@
>
> #include <vdso/const.h>
>
> +/*
> + * This returns a constant expression while determining if an argument is
> + * a constant expression, most importantly without evaluating the argument.
> + * Glory to Martin Uecker <[email protected]>
> + */
> +#define __is_constexpr(x) \
> + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))

Boggle.

Could someone please sometime enhance that comment a bit? What need
does this thing satisfy and how on earth does it work?

2021-05-21 20:12:50

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] linux/bits.h: Fix compilation error with GENMASK

On Tue, May 11, 2021 at 11:41 PM Rikard Falkeborn
<[email protected]> wrote:
>
> GENMASK() has an input check which uses __builtin_choose_expr() to enable
> a compile time sanity check of its inputs if they are known at compile
> time. However, it turns out that __builtin_constant_p() does not always
> return a compile time constant [0]. It was thought this problem was fixed
> with gcc 4.9 [1], but apparently this is not the case [2].
>
> Switch to use __is_constexpr() instead which always returns a compile
> time constant, regardless of its inputs.
>
> [0]: https://lore.kernel.org/lkml/[email protected]
> [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> [2]: https://lore.kernel.org/lkml/[email protected]

Thanks for fixing this issue!
Since there is a consensus about the place, I have no objection either.
Reviewed-by: Andy Shevchenko <[email protected]>

> Reported-by: Tetsuo Handa <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> Feedback on placing __is_constexpr() in const.h is welcome, at least the
> name is appropriate...
>
> include/linux/bits.h | 2 +-
> include/linux/const.h | 8 ++++++++
> include/linux/minmax.h | 10 ++--------
> tools/include/linux/bits.h | 2 +-
> tools/include/linux/const.h | 8 ++++++++
> 5 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 7f475d59a097..87d112650dfb 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -22,7 +22,7 @@
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> + __is_constexpr((l) > (h)), (l) > (h), 0)))
> #else
> /*
> * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> diff --git a/include/linux/const.h b/include/linux/const.h
> index 81b8aae5a855..435ddd72d2c4 100644
> --- a/include/linux/const.h
> +++ b/include/linux/const.h
> @@ -3,4 +3,12 @@
>
> #include <vdso/const.h>
>
> +/*
> + * This returns a constant expression while determining if an argument is
> + * a constant expression, most importantly without evaluating the argument.
> + * Glory to Martin Uecker <[email protected]>
> + */
> +#define __is_constexpr(x) \
> + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> +
> #endif /* _LINUX_CONST_H */
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index c0f57b0c64d9..5433c08fcc68 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -2,6 +2,8 @@
> #ifndef _LINUX_MINMAX_H
> #define _LINUX_MINMAX_H
>
> +#include <linux/const.h>
> +
> /*
> * min()/max()/clamp() macros must accomplish three things:
> *
> @@ -17,14 +19,6 @@
> #define __typecheck(x, y) \
> (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>
> -/*
> - * This returns a constant expression while determining if an argument is
> - * a constant expression, most importantly without evaluating the argument.
> - * Glory to Martin Uecker <[email protected]>
> - */
> -#define __is_constexpr(x) \
> - (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> -
> #define __no_side_effects(x, y) \
> (__is_constexpr(x) && __is_constexpr(y))
>
> diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> index 7f475d59a097..87d112650dfb 100644
> --- a/tools/include/linux/bits.h
> +++ b/tools/include/linux/bits.h
> @@ -22,7 +22,7 @@
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> + __is_constexpr((l) > (h)), (l) > (h), 0)))
> #else
> /*
> * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> diff --git a/tools/include/linux/const.h b/tools/include/linux/const.h
> index 81b8aae5a855..435ddd72d2c4 100644
> --- a/tools/include/linux/const.h
> +++ b/tools/include/linux/const.h
> @@ -3,4 +3,12 @@
>
> #include <vdso/const.h>
>
> +/*
> + * This returns a constant expression while determining if an argument is
> + * a constant expression, most importantly without evaluating the argument.
> + * Glory to Martin Uecker <[email protected]>
> + */
> +#define __is_constexpr(x) \
> + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> +
> #endif /* _LINUX_CONST_H */
> --
> 2.31.1
>


--
With Best Regards,
Andy Shevchenko