* [PATCH] bitops.h: Widen BIT macro to support 64-bit types [not found] ` <20101014042409.GB29342@aftab> @ 2010-10-14 10:58 ` Borislav Petkov 2010-10-14 11:12 ` Matthew Wilcox 2010-10-14 15:03 ` Linus Torvalds 0 siblings, 2 replies; 6+ messages in thread From: Borislav Petkov @ 2010-10-14 10:58 UTC (permalink / raw) To: Randy Dunlap, Linus Torvalds; +Cc: lkml, Doug Thompson, akpm, linux-arch > 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types 2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov @ 2010-10-14 11:12 ` Matthew Wilcox 2010-10-14 12:12 ` Borislav Petkov 2010-10-14 15:03 ` Linus Torvalds 1 sibling, 1 reply; 6+ messages in thread From: Matthew Wilcox @ 2010-10-14 11:12 UTC (permalink / raw) To: Borislav Petkov Cc: Randy Dunlap, Linus Torvalds, lkml, Doug Thompson, akpm, linux-arch 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." ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types 2010-10-14 11:12 ` Matthew Wilcox @ 2010-10-14 12:12 ` Borislav Petkov 2010-10-14 14:27 ` Matthew Wilcox 0 siblings, 1 reply; 6+ messages in thread From: Borislav Petkov @ 2010-10-14 12:12 UTC (permalink / raw) To: Matthew Wilcox Cc: Randy Dunlap, Linus Torvalds, lkml, Doug Thompson, akpm, linux-arch@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types 2010-10-14 12:12 ` Borislav Petkov @ 2010-10-14 14:27 ` Matthew Wilcox 0 siblings, 0 replies; 6+ messages in thread From: Matthew Wilcox @ 2010-10-14 14:27 UTC (permalink / raw) To: Borislav Petkov Cc: Randy Dunlap, Linus Torvalds, lkml, Doug Thompson, akpm, linux-arch@vger.kernel.org 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." ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types 2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov 2010-10-14 11:12 ` Matthew Wilcox @ 2010-10-14 15:03 ` Linus Torvalds 2010-10-14 15:36 ` Borislav Petkov 1 sibling, 1 reply; 6+ messages in thread From: Linus Torvalds @ 2010-10-14 15:03 UTC (permalink / raw) To: Borislav Petkov; +Cc: Randy Dunlap, lkml, Doug Thompson, akpm, linux-arch 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bitops.h: Widen BIT macro to support 64-bit types 2010-10-14 15:03 ` Linus Torvalds @ 2010-10-14 15:36 ` Borislav Petkov 0 siblings, 0 replies; 6+ messages in thread From: Borislav Petkov @ 2010-10-14 15:36 UTC (permalink / raw) To: Linus Torvalds; +Cc: Randy Dunlap, lkml, Doug Thompson, akpm, linux-arch 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-14 15:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20101013131057.88238be6.randy.dunlap@oracle.com>
[not found] ` <20101014042409.GB29342@aftab>
2010-10-14 10:58 ` [PATCH] bitops.h: Widen BIT macro to support 64-bit types Borislav Petkov
2010-10-14 11:12 ` Matthew Wilcox
2010-10-14 12:12 ` Borislav Petkov
2010-10-14 14:27 ` Matthew Wilcox
2010-10-14 15:03 ` Linus Torvalds
2010-10-14 15:36 ` Borislav Petkov
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).