Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572524 > unrolled thread

[PATCH] log2: make order_base_2() behave correctly on const input value zero

Started byArd Biesheuvel <ard.biesheuvel@linaro.org>
First post2017-02-02 18:10 +0100
Last post2017-02-02 22:00 +0100
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] log2: make order_base_2() behave correctly on const input value zero Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-02 18:10 +0100
    Re: [PATCH] log2: make order_base_2() behave correctly on const  input value zero kbuild test robot <lkp@intel.com> - 2017-02-02 19:00 +0100
    Re: [PATCH] log2: make order_base_2() behave correctly on const input  value zero Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-02 21:50 +0100
      Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-02 22:00 +0100

#1572524 — [PATCH] log2: make order_base_2() behave correctly on const input value zero

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2017-02-02 18:10 +0100
Subject[PATCH] log2: make order_base_2() behave correctly on const input value zero
Message-ID<t6t3r-6OU-17@gated-at.bofh.it>
The function order_base_2() is defined (according to the comment block)
as returning zero on input zero, but subsequently passes the input into
roundup_pow_of_two(), which is explicitly undefined for input zero.

This has gone unnoticed until now, but optimization passes in GCC 7 may
produce constant folded function instances where a constant value of zero
is passed into order_base_2(), resulting in link errors against the
deliberately undefined '____ilog2_NaN'.

So update order_base_2() to adhere to its own documented interface.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

This is a followup to Will's message 'Build failure with v4.9-rc1 and GCC
trunk -- compiler weirdness' which can be found here:

  http://marc.info/?l=linux-kernel&m=147672952517795&w=2

 include/linux/log2.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d91e6a..d3fe63b12e96 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -203,6 +203,17 @@ 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 > 1 ? ilog2(n - 1) + 1 : 0;
+}
 
+#define order_base_2(n)				\
+(						\
+	__builtin_constant_p(n) ? (		\
+		((n) == 0 || (n) == 1) ? 0 :	\
+		ilog2((n) - 1) + 1) :		\
+	__order_base_2(n)			\
+)
 #endif /* _LINUX_LOG2_H */
-- 
2.7.4

[toc] | [next] | [standalone]

#1572560 — Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero

Fromkbuild test robot <lkp@intel.com>
Date2017-02-02 19:00 +0100
SubjectRe: [PATCH] log2: make order_base_2() behave correctly on const input value zero
Message-ID<t6tPQ-77C-21@gated-at.bofh.it>
In reply to#1572524

[Multipart message — attachments visible in raw view] — view raw

Hi Ard,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.10-rc6 next-20170202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ard-Biesheuvel/log2-make-order_base_2-behave-correctly-on-const-input-value-zero/20170203-012201
config: i386-defconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/drm_bufs.c:36:0:
   drivers/gpu/drm/drm_bufs.c: In function 'drm_addmap_core':
>> drivers/gpu/drm/drm_bufs.c:239:13: warning: format '%d' expects argument of type 'int', but argument 5 has type 'long unsigned int' [-Wformat=]
      DRM_DEBUG("%lu %d %p\n",
                ^
   include/drm/drmP.h:222:38: note: in definition of macro 'DRM_DEBUG'
     drm_printk(KERN_DEBUG, DRM_UT_CORE, fmt, ##__VA_ARGS__)
                                         ^~~

vim +239 drivers/gpu/drm/drm_bufs.c

^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  223  		break;
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  224  	case _DRM_SHM:
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  225  		list = drm_find_matching_map(dev, map);
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  226  		if (list != NULL) {
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  227  			if(list->map->size != map->size) {
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  228  				DRM_DEBUG("Matching maps of type %d with "
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  229  					  "mismatched sizes, (%ld vs %ld)\n",
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  230  					  map->type, map->size, list->map->size);
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  231  				list->map->size = map->size;
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  232  			}
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  233  
9a298b2ac drivers/gpu/drm/drm_bufs.c  Eric Anholt      2009-03-24  234  			kfree(map);
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  235  			*maplist = list;
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  236  			return 0;
54ba2f76e drivers/char/drm/drm_bufs.c Dave Airlie      2007-02-10  237  		}
f239b7b0c drivers/char/drm/drm_bufs.c Thomas Hellstrom 2007-01-08  238  		map->handle = vmalloc_user(map->size);
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16 @239  		DRM_DEBUG("%lu %d %p\n",
04420c9c6 drivers/gpu/drm/drm_bufs.c  Daniel Vetter    2013-07-10  240  			  map->size, order_base_2(map->size), map->handle);
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  241  		if (!map->handle) {
9a298b2ac drivers/gpu/drm/drm_bufs.c  Eric Anholt      2009-03-24  242  			kfree(map);
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  243  			return -ENOMEM;
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  244  		}
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  245  		map->offset = (unsigned long)map->handle;
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  246  		if (map->flags & _DRM_CONTAINS_LOCK) {
^1da177e4 drivers/char/drm/drm_bufs.c Linus Torvalds   2005-04-16  247  			/* Prevent a 2nd X Server from creating a 2nd lock */

:::::: The code at line 239 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1572743 — Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2017-02-02 21:50 +0100
SubjectRe: [PATCH] log2: make order_base_2() behave correctly on const input value zero
Message-ID<t6wum-qe-13@gated-at.bofh.it>
In reply to#1572524
On Thu, Feb 2, 2017 at 9:04 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> So update order_base_2() to adhere to its own documented interface.

Ok, looks like you screwed up the types according to the build server.

Making the return type "unsigned long" is slightly excessive. If you
need an unsigned long to describe the log2 of a number in the kernel,
you're playing with numbers that are *way* too big.

The old code used "ilog2()", which returns an int.

Which is plenty big enough of a type.

                Linus

[toc] | [prev] | [next] | [standalone]

#1572748

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2017-02-02 22:00 +0100
Message-ID<t6wE2-ua-9@gated-at.bofh.it>
In reply to#1572743
> On 2 Feb 2017, at 20:49, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> On Thu, Feb 2, 2017 at 9:04 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> 
>> So update order_base_2() to adhere to its own documented interface.
> 
> Ok, looks like you screwed up the types according to the build server.
> 
> Making the return type "unsigned long" is slightly excessive. If you
> need an unsigned long to describe the log2 of a number in the kernel,
> you're playing with numbers that are *way* too big.
> 
> The old code used "ilog2()", which returns an int.
> 

Yeah I cloned roundup_pow_of_2, and failed to spot that ilog2 returns an int. I sent out the v2 already

> Which is plenty big enough of a type.
> 
>                Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web