diff mbox

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

Message ID 1486055052-6442-1-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Feb. 2, 2017, 5:04 p.m. UTC 
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(-)

-- 
2.7.4

Comments

kernel test robot Feb. 2, 2017, 5:55 p.m. UTC | #1 
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
kernel test robot Feb. 2, 2017, 7:17 p.m. UTC | #2 
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
Linus Torvalds Feb. 2, 2017, 8:49 p.m. UTC | #3 
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
Ard Biesheuvel Feb. 2, 2017, 8:51 p.m. UTC | #4 
> 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
diff mbox

Patch

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 */