Build failure with v4.9-rc1 and GCC trunk -- compiler weirdness

On 1 February 2017 at 19:49, Joe Perches [off-list ref] wrote:

On Wed, 2017-02-01 at 19:31 +0000, Ard Biesheuvel wrote:

quoted

On 1 February 2017 at 19:04, Joe Perches [off-list ref] wrote:

quoted

On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote:

quoted

On 1 February 2017 at 17:36, Ard Biesheuvel [off-list ref] wrote:

quoted

I still think order_base_2() is broken, since it may invoke
roundup_pow_of_two() with an input value that is documented as
producing undefined output. I would argue that the below is the
correct fix.

diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d91e6a..46523731bec0 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h

@@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
  *  ... and so on.
  */

-#define order_base_2(n) ilog2(roundup_pow_of_two(n))
+static inline __attribute__((__const__))
+unsigned long __order_base_2(unsigned long n)
+{
+       return n ? 1UL << fls_long(n - 1) : 1;
+}
+
+#define order_base_2(n)                                \
+(                                              \
+       __builtin_constant_p(n) ? (             \
+               ((n) < 2) ? (n) :               \
+               ilog2((n) - 1) + 1) :           \
+       ilog2(__order_base_2(n))                \
+ )

 #endif /* _LINUX_LOG2_H */

Actually, there is a still a redundant shift/fls() in there, this is
even simpler:

diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d91e6a..4741534bd7af 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h

@@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
  *  ... and so on.
  */

-#define order_base_2(n) ilog2(roundup_pow_of_two(n))
+static inline __attribute__((__const__))

commonly __attribute_const__

... except in <linux/ilog2.h>, which probably predates that.

quoted

quoted

+unsigned long __order_base_2(unsigned long n)
+{
+       return n > 1 ? ilog2(n - 1) + 1 : 0;
+}
+
+#define order_base_2(n)                                \
+(                                              \
+       __builtin_constant_p(n) ? (             \
+               ((n) < 2) ? (n) :               \
+               ilog2((n) - 1) + 1) :           \
+       __order_base_2(n)                       \
+ )

Does this work properly when n is a signed negative value?

No, but order_base_2() is explicitly only defined for inputs [0, ->),

where?

The comment describes it as follows

 /**
  * order_base_2 - calculate the (rounded up) base 2 order of the argument
  * @n: parameter
  *
  * The first few values calculated by this routine:
  *  ob2(0) = 0
  *  ob2(1) = 0
  *  ob2(2) = 1
  *  ob2(3) = 2
  *  ob2(4) = 2
  *  ob2(5) = 3
  *  ... and so on.
  */

i.e., it defines the output for inputs 0, 1, 2, 3, 4, 5, ..., and not
for negative inputs, hence undefined.

quoted

so its behavior for negative inputs is best left undefined.

Or maybe add a BUILD_BUG_ON something like:

#define order_base_2(n)                                                 \
({                                                                      \
        typeof(n) _n = n;                                               \
        BUILD_BUG_ON(__builtin_constant_p(_n) && _n < 0);               \
        __builtin_constant_p(_n) ? (_n < 2 ? _n : ilog2((_n) - 1) + 1)) \
                                 : __order_base_2(_n);                  \
})

This would interfere with the ability to use order_base_2() in
initializers for global variables.