* [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 related [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;
as well as URLs for NNTP newsgroup(s).