[PATCH] bitops.h: Widen BIT macro to support 64-bit types

[PATCH] bitops.h: Widen BIT macro to support 64-bit types

[Borislav Petkov]

> From: Randy Dunlap <randy.dunlap@oracle.com>
> Date: Wed, Oct 13, 2010 at 04:10:57PM -0400
> 
> > The BIT() macro works on unsigned longs, but this warning happens
> > on i386 (X86_32), where UL is 32 bits and this value needs to be
> > 64 bits, so use open-coded ULL.
> > 
> > drivers/edac/mce_amd.c:262: warning: left shift count >= width of type
> 
> Ok, so BIT() should be fixed to work with the largest type available,
> IMHO. Let me cook up something.

Maybe something like the following. Build-tested with the crosstool
(http://www.kernel.org/pub/tools/crosstool) on the following arches:
alpha blackfin cris hppa64 ia64 mips64 sparc.

Any objections?

--
From: Borislav Petkov <Borislav.Petkov@amd.com>
Date: Thu, 14 Oct 2010 12:00:17 +0200
Subject: [PATCH] bitops.h: Widen BIT macro to support 64-bit types

Nowadays, every arch defines an unsigned 64-bit type using either one
of the include/asm-generic/int-l{,l}64.h headers. Thus, make the BIT()
macro return that 64-bit type by default. This makes sense on x86 when
manipulating MSR values and prevents overflow errors on 32-bit arches,
for example.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 include/linux/bitops.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index fc68053..7b9170b 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -3,7 +3,7 @@
 #include <asm/types.h>
 
 #ifdef	__KERNEL__
-#define BIT(nr)			(1UL << (nr))
+#define BIT(nr)			(U64_C(1) << (nr))
 #define BIT_MASK(nr)		(1UL << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BITS_PER_BYTE		8
-- 
1.7.3.1.50.g1e633

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types

[Matthew Wilcox]

On Thu, Oct 14, 2010 at 12:58:55PM +0200, Borislav Petkov wrote:
>  #ifdef	__KERNEL__
> -#define BIT(nr)			(1UL << (nr))
> +#define BIT(nr)			(U64_C(1) << (nr))

Why not just use 1ULL instead?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types

[Borislav Petkov]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, Oct 14, 2010 at 07:12:13AM -0400

> On Thu, Oct 14, 2010 at 12:58:55PM +0200, Borislav Petkov wrote:
> >  #ifdef	__KERNEL__
> > -#define BIT(nr)			(1UL << (nr))
> > +#define BIT(nr)			(U64_C(1) << (nr))
> 
> Why not just use 1ULL instead?

Wanted to be __ASSEMBLY__ safe.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types

[Matthew Wilcox]

On Thu, Oct 14, 2010 at 02:12:30PM +0200, Borislav Petkov wrote:
> From: Matthew Wilcox <matthew@wil.cx>
> Date: Thu, Oct 14, 2010 at 07:12:13AM -0400
> 
> > On Thu, Oct 14, 2010 at 12:58:55PM +0200, Borislav Petkov wrote:
> > >  #ifdef	__KERNEL__
> > > -#define BIT(nr)			(1UL << (nr))
> > > +#define BIT(nr)			(U64_C(1) << (nr))
> > 
> > Why not just use 1ULL instead?
> 
> Wanted to be __ASSEMBLY__ safe.

Admirable, but the entire file is __ASSEMBLY__ unsafe at this point,
so I don't see the point.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types

[Linus Torvalds]

On Thu, Oct 14, 2010 at 3:58 AM, Borislav Petkov <bp@amd64.org> wrote:
>>
>> Ok, so BIT() should be fixed to work with the largest type available,
>> IMHO. Let me cook up something.
>
> Maybe something like the following. Build-tested with the crosstool
> (http://www.kernel.org/pub/tools/crosstool) on the following arches:
> alpha blackfin cris hppa64 ia64 mips64 sparc.
>
> Any objections?

Yeah. I object. I have no idea what this will change for everything
else that expects bitops to work on unsigned long values.

I really think that the bug is not in the BIT() definition, but in the
use. If somebody wants a non-unsigned-long bit field, they had better
not use bitops.h.

And no, just changing the BIT() macro to return a 64-bit value is
_not_ trivially safe. Due to C type rules, now all arithmetic using
BIT() will suddenly be 64-bit, which is often *much* slower, and can
introduce real bugs.

On many architectures, a 64-bit non-constant shift will even end up
being a function call. And if the thing is used in a varargs function,
the argument layout will be totally different. We've also had several
issues with 64-bit types and switch() statements, for example. And a
quick grep for '\<BIT(' shows that non-constant cases are not unheard
of, and there's a lot of random use where it is not at all obvious
that it's safe (because it's used for defining other defines).

So no. I do not think BIT() should be 64-bit. It's "unsigned long".
Look at all the other things around it, and look at all the historical
uses.

                                    Linus

Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types

[Borislav Petkov]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, Oct 14, 2010 at 08:03:17AM -0700

> On Thu, Oct 14, 2010 at 3:58 AM, Borislav Petkov <bp@amd64.org> wrote:
> >>
> >> Ok, so BIT() should be fixed to work with the largest type available,
> >> IMHO. Let me cook up something.
> >
> > Maybe something like the following. Build-tested with the crosstool
> > (http://www.kernel.org/pub/tools/crosstool) on the following arches:
> > alpha blackfin cris hppa64 ia64 mips64 sparc.
> >
> > Any objections?
> 
> Yeah. I object. I have no idea what this will change for everything
> else that expects bitops to work on unsigned long values.
> 
> I really think that the bug is not in the BIT() definition, but in the
> use. If somebody wants a non-unsigned-long bit field, they had better
> not use bitops.h.
> 
> And no, just changing the BIT() macro to return a 64-bit value is
> _not_ trivially safe. Due to C type rules, now all arithmetic using
> BIT() will suddenly be 64-bit, which is often *much* slower, and can
> introduce real bugs.
> 
> On many architectures, a 64-bit non-constant shift will even end up
> being a function call. And if the thing is used in a varargs function,
> the argument layout will be totally different. We've also had several
> issues with 64-bit types and switch() statements, for example. And a
> quick grep for '\<BIT(' shows that non-constant cases are not unheard
> of, and there's a lot of random use where it is not at all obvious
> that it's safe (because it's used for defining other defines).

Concerning safety, I actually had a version which did check the bit
number supplied as an arg for overflowing but this failed when using
BIT() in struct initializers:

	.struct_member = { BIT(bla) }

But thanks for the detailed explanation! This makes perfect sense; it
was too much wishful thinking on my part to assume that a ULL BIT()
would be fine after checking that all arches support the unsigned 64-bit
type.

I'm much better off with a local BIT_64() or similar, definition.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632