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

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

[Rikard Falkeborn]

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/42b4342b-aefc-a16a-0d43-9f9c0d63ba7a@rasmusvillemoes.dk
[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
[2]: https://lore.kernel.org/lkml/1ac7bbc2-45d9-26ed-0b33-bf382b8d858b@I-love.SAKURA.ne.jp

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
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 <Martin.Uecker@med.uni-goettingen.de>
+ */
+#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 <Martin.Uecker@med.uni-goettingen.de>
- */
-#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 <Martin.Uecker@med.uni-goettingen.de>
+ */
+#define __is_constexpr(x) \
+	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
+
 #endif /* _LINUX_CONST_H */
-- 
2.31.1

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

[Ard Biesheuvel]

(+ Arnd)

On Tue, 11 May 2021 at 22:37, Rikard Falkeborn
<rikard.falkeborn@gmail.com> 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/42b4342b-aefc-a16a-0d43-9f9c0d63ba7a@rasmusvillemoes.dk
> [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> [2]: https://lore.kernel.org/lkml/1ac7bbc2-45d9-26ed-0b33-bf382b8d858b@I-love.SAKURA.ne.jp
>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> 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 <Martin.Uecker@med.uni-goettingen.de>
> + */
> +#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 <Martin.Uecker@med.uni-goettingen.de>
> - */
> -#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 <Martin.Uecker@med.uni-goettingen.de>
> + */
> +#define __is_constexpr(x) \
> +       (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> +
>  #endif /* _LINUX_CONST_H */
> --
> 2.31.1
>

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

[Rikard Falkeborn]

On Wed, May 12, 2021 at 02:53:27PM +0200, Ard Biesheuvel wrote:
> (+ Arnd)
> 
> On Tue, 11 May 2021 at 22:37, Rikard Falkeborn
> <rikard.falkeborn@gmail.com> 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/42b4342b-aefc-a16a-0d43-9f9c0d63ba7a@rasmusvillemoes.dk
> > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> > [2]: https://lore.kernel.org/lkml/1ac7bbc2-45d9-26ed-0b33-bf382b8d858b@I-love.SAKURA.ne.jp
> >
> > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> > ---
> > 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 <Martin.Uecker@med.uni-goettingen.de>
> > + */
> > +#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 <Martin.Uecker@med.uni-goettingen.de>
> > - */
> > -#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 <Martin.Uecker@med.uni-goettingen.de>
> > + */
> > +#define __is_constexpr(x) \
> > +       (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > +
> >  #endif /* _LINUX_CONST_H */
> > --
> > 2.31.1
> >

Friendly ping.

Rikard

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

[Yury Norov]

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
> > <rikard.falkeborn@gmail.com> 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/42b4342b-aefc-a16a-0d43-9f9c0d63ba7a@rasmusvillemoes.dk
> > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> > > [2]: https://lore.kernel.org/lkml/1ac7bbc2-45d9-26ed-0b33-bf382b8d858b@I-love.SAKURA.ne.jp
> > >
> > > Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> > > ---
> > > 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 <Martin.Uecker@med.uni-goettingen.de>
> > > + */
> > > +#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 <Martin.Uecker@med.uni-goettingen.de>
> > > - */
> > > -#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 <Martin.Uecker@med.uni-goettingen.de>
> > > + */
> > > +#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?

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

[Arnd Bergmann]

On Thu, May 20, 2021 at 9:44 PM Yury Norov <yury.norov@gmail.com> 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 <Martin.Uecker@med.uni-goettingen.de>
> > > > + */
> > > > +#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 <arnd@arndb.de>

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

[Andrew Morton]

On Tue, 11 May 2021 22:37:15 +0200 Rikard Falkeborn <rikard.falkeborn@gmail.com> 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 <Martin.Uecker@med.uni-goettingen.de>
> + */
> +#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?

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

[Andy Shevchenko]

On Tue, May 11, 2021 at 11:41 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> 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/42b4342b-aefc-a16a-0d43-9f9c0d63ba7a@rasmusvillemoes.dk
> [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> [2]: https://lore.kernel.org/lkml/1ac7bbc2-45d9-26ed-0b33-bf382b8d858b@I-love.SAKURA.ne.jp

Thanks for fixing this issue!
Since there is a consensus about the place, I have no objection either.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> 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 <Martin.Uecker@med.uni-goettingen.de>
> + */
> +#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 <Martin.Uecker@med.uni-goettingen.de>
> - */
> -#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 <Martin.Uecker@med.uni-goettingen.de>
> + */
> +#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

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

[Andy Shevchenko]

On Thu, May 20, 2021 at 01:41:12PM -0700, Andrew Morton wrote:
> On Tue, 11 May 2021 22:37:15 +0200 Rikard Falkeborn <rikard.falkeborn@gmail.com> 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 <Martin.Uecker@med.uni-goettingen.de>
> > + */
> > +#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