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

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH] log2: make order_base_2() behave correctly on const input value zero
@ 2017-02-02 17:04 Ard Biesheuvel
  2017-02-02 17:55 ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2017-02-02 17:04 UTC (permalink / raw)
  To: linux-kernel, labbott, will.deacon, sboyd, gregory.clement,
	james.greenhalgh, mingo, peterz, markus, akpm
  Cc: linux-arm-kernel, torvalds, joe, Ard Biesheuvel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero
  2017-02-02 17:04 [PATCH] log2: make order_base_2() behave correctly on const input value zero Ard Biesheuvel
@ 2017-02-02 17:55 ` kbuild test robot
  2017-02-02 19:17 ` kbuild test robot
  2017-02-02 20:49 ` Linus Torvalds
  2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2017-02-02 17:55 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: kbuild-all, linux-kernel, labbott, will.deacon, sboyd,
	gregory.clement, james.greenhalgh, mingo, peterz, markus, akpm,
	linux-arm-kernel, torvalds, joe, Ard Biesheuvel

[-- Attachment #1: Type: text/plain, Size: 3980 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25436 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero
  2017-02-02 17:04 [PATCH] log2: make order_base_2() behave correctly on const input value zero Ard Biesheuvel
  2017-02-02 17:55 ` kbuild test robot
@ 2017-02-02 19:17 ` kbuild test robot
  2017-02-02 20:49 ` Linus Torvalds
  2 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2017-02-02 19:17 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: kbuild-all, linux-kernel, labbott, will.deacon, sboyd,
	gregory.clement, james.greenhalgh, mingo, peterz, markus, akpm,
	linux-arm-kernel, torvalds, joe, Ard Biesheuvel

[-- Attachment #1: Type: text/plain, Size: 3679 bytes --]

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: x86_64-randconfig-s2-02030209 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_bufs.c: In function 'drm_addmap_core':
>> drivers/gpu/drm/drm_bufs.c:239: warning: format '%d' expects type 'int', but argument 5 has type 'long unsigned int'

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30294 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero
  2017-02-02 17:04 [PATCH] log2: make order_base_2() behave correctly on const input value zero Ard Biesheuvel
  2017-02-02 17:55 ` kbuild test robot
  2017-02-02 19:17 ` kbuild test robot
@ 2017-02-02 20:49 ` Linus Torvalds
  2017-02-02 20:51   ` Ard Biesheuvel
  2 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2017-02-02 20:49 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Linux Kernel Mailing List, Laura Abbott, Will Deacon,
	Stephen Boyd, Gregory Clement, james.greenhalgh, Ingo Molnar,
	Peter Zijlstra, Markus Trippelsdorf, Andrew Morton,
	linux-arm-kernel, Joe Perches

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make order_base_2() behave correctly on const input value zero
  2017-02-02 20:49 ` Linus Torvalds
@ 2017-02-02 20:51   ` Ard Biesheuvel
  0 siblings, 0 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2017-02-02 20:51 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Linux Kernel Mailing List, Laura Abbott, Will Deacon,
	Stephen Boyd, Gregory Clement, james.greenhalgh, Ingo Molnar,
	Peter Zijlstra, Markus Trippelsdorf, Andrew Morton,
	linux-arm-kernel, Joe Perches


> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-02-02 20:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-02 17:04 [PATCH] log2: make order_base_2() behave correctly on const input value zero Ard Biesheuvel
2017-02-02 17:55 ` kbuild test robot
2017-02-02 19:17 ` kbuild test robot
2017-02-02 20:49 ` Linus Torvalds
2017-02-02 20:51   ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®