* RE: [PATCH 5/6] fix warning on test_ti_thread_flag()
@ 2006-01-25 17:08 Chen, Kenneth W
2006-01-25 17:19 ` Geert Uytterhoeven
0 siblings, 1 reply; 9+ messages in thread
From: Chen, Kenneth W @ 2006-01-25 17:08 UTC (permalink / raw)
To: Geert Uytterhoeven, Akinobu Mita
Cc: Linux Kernel Development, Richard Henderson, Ivan Kokshaysky,
Russell King, Ian Molton, dev-etrax, David Howells,
Yoshinori Sato, Linus Torvalds, linux-ia64, Hirokazu Takata,
linux-m68k, Greg Ungerer, Linux/MIPS Development, parisc-linux,
Linux/PPC Development, linux390, linuxsh-dev,
linuxsh-shmedia-dev, sparclinux, ultralinux, Miles Bader,
Andi Kleen, Chris Zankel
Geert Uytterhoeven wrote on Wednesday, January 25, 2006 4:29 AM
> On Wed, 25 Jan 2006, Akinobu Mita wrote:
> > If the arechitecture is
> > - BITS_PER_LONG == 64
> > - struct thread_info.flag 32 is bits
> > - second argument of test_bit() was void *
> >
> > Then compiler print error message on test_ti_thread_flags()
> > in include/linux/thread_info.h
> >
> > Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
> > ---
> > thread_info.h | 2 +-
> > 1 files changed, 1 insertion(+), 1 deletion(-)
> >
> > Index: 2.6-git/include/linux/thread_info.h
> > ===================================================================
> > --- 2.6-git.orig/include/linux/thread_info.h 2006-01-25
19:07:12.000000000 +0900
> > +++ 2.6-git/include/linux/thread_info.h 2006-01-25
19:14:26.000000000 +0900
> > @@ -49,7 +49,7 @@
> >
> > static inline int test_ti_thread_flag(struct thread_info *ti, int
flag)
> > {
> > - return test_bit(flag,&ti->flags);
> > + return test_bit(flag, (void *)&ti->flags);
> > }
>
> This is not safe. The bitops are defined to work on unsigned long
only, so
> flags should be changed to unsigned long instead, or you should use a
> temporary.
>
> Affected platforms:
> - alpha: flags is unsigned int
> - ia64, sh, x86_64: flags is __u32
>
> The only affected 64-platforms are little endian, so it will silently
work
> after your change, though...
I thought test_bit can operate on array beyond unsigned long.
It's perfectly legitimate to do: test_bit(999, bit_array) as
long as bit_array is indeed big enough to hold 999 bits. It
is the responsibility of the caller to make sure that the
underlying array is big enough for the bit that is being tested.
I don't think you need to change the flags size.
- Ken
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 17:08 [PATCH 5/6] fix warning on test_ti_thread_flag() Chen, Kenneth W
@ 2006-01-25 17:19 ` Geert Uytterhoeven
2006-01-25 20:02 ` Chen, Kenneth W
0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2006-01-25 17:19 UTC (permalink / raw)
To: Chen, Kenneth W
Cc: Akinobu Mita, Linux Kernel Development, Richard Henderson,
Ivan Kokshaysky, Russell King, Ian Molton, dev-etrax,
David Howells, Yoshinori Sato, Linus Torvalds, linux-ia64,
Hirokazu Takata, linux-m68k, Greg Ungerer,
Linux/MIPS Development, parisc-linux, Linux/PPC Development,
linux390, linuxsh-dev, linuxsh-shmedia-dev, sparclinux,
ultralinux, Miles Bader, Andi Kleen, Chris Zankel
On Wed, 25 Jan 2006, Chen, Kenneth W wrote:
> Geert Uytterhoeven wrote on Wednesday, January 25, 2006 4:29 AM
> > On Wed, 25 Jan 2006, Akinobu Mita wrote:
> > > If the arechitecture is
> > > - BITS_PER_LONG == 64
> > > - struct thread_info.flag 32 is bits
> > > - second argument of test_bit() was void *
> > >
> > > Then compiler print error message on test_ti_thread_flags()
> > > in include/linux/thread_info.h
> > >
> > > Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
> > > ---
> > > thread_info.h | 2 +-
> > > 1 files changed, 1 insertion(+), 1 deletion(-)
> > >
> > > Index: 2.6-git/include/linux/thread_info.h
> > > ===================================================================
> > > --- 2.6-git.orig/include/linux/thread_info.h 2006-01-25
> 19:07:12.000000000 +0900
> > > +++ 2.6-git/include/linux/thread_info.h 2006-01-25
> 19:14:26.000000000 +0900
> > > @@ -49,7 +49,7 @@
> > >
> > > static inline int test_ti_thread_flag(struct thread_info *ti, int
> flag)
> > > {
> > > - return test_bit(flag,&ti->flags);
> > > + return test_bit(flag, (void *)&ti->flags);
> > > }
> >
> > This is not safe. The bitops are defined to work on unsigned long
> only, so
> > flags should be changed to unsigned long instead, or you should use a
> > temporary.
> >
> > Affected platforms:
> > - alpha: flags is unsigned int
> > - ia64, sh, x86_64: flags is __u32
> >
> > The only affected 64-platforms are little endian, so it will silently
> work
> > after your change, though...
>
> I thought test_bit can operate on array beyond unsigned long.
> It's perfectly legitimate to do: test_bit(999, bit_array) as
> long as bit_array is indeed big enough to hold 999 bits. It
> is the responsibility of the caller to make sure that the
> underlying array is big enough for the bit that is being tested.
Yes, it can operate on arrays of unsigned long.
> I don't think you need to change the flags size.
Passing a pointer to a 32-bit entity to a function that takes a pointer to a
64-bit entity is a classical endianness bug. So it's better to change it,
before people copy the code to a big endian platform.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 17:19 ` Geert Uytterhoeven
@ 2006-01-25 20:02 ` Chen, Kenneth W
2006-01-26 3:50 ` Akinobu Mita
0 siblings, 1 reply; 9+ messages in thread
From: Chen, Kenneth W @ 2006-01-25 20:02 UTC (permalink / raw)
To: 'Geert Uytterhoeven'
Cc: Akinobu Mita, Linux Kernel Development, linux-ia64, linux-m68k,
parisc-linux, Linux/PPC Development, linux390, linuxsh-dev,
sparclinux, ultralinux, Andi Kleen
Geert Uytterhoeven wrote on Wednesday, January 25, 2006 9:19 AM
> > I don't think you need to change the flags size.
>
> Passing a pointer to a 32-bit entity to a function that takes a
> pointer to a 64-bit entity is a classical endianness bug. So it's
> better to change it, before people copy the code to a big endian
> platform.
Well, x86-64 and linux-ia64 both use little endian. I don't
understand why you are barking at us with big endian issue.
- Ken
Side-note: cc list trimmed.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 20:02 ` Chen, Kenneth W
@ 2006-01-26 3:50 ` Akinobu Mita
2006-01-26 4:12 ` Paul Mackerras
0 siblings, 1 reply; 9+ messages in thread
From: Akinobu Mita @ 2006-01-26 3:50 UTC (permalink / raw)
To: Chen, Kenneth W
Cc: 'Geert Uytterhoeven',
Linux Kernel Development, linux-ia64, linux-m68k, parisc-linux,
Linux/PPC Development, linux390, linuxsh-dev, sparclinux,
ultralinux, Andi Kleen
On Wed, Jan 25, 2006 at 12:02:21PM -0800, Chen, Kenneth W wrote:
> Geert Uytterhoeven wrote on Wednesday, January 25, 2006 9:19 AM
> > > I don't think you need to change the flags size.
> >
> > Passing a pointer to a 32-bit entity to a function that takes a
> > pointer to a 64-bit entity is a classical endianness bug. So it's
> > better to change it, before people copy the code to a big endian
> > platform.
>
> Well, x86-64 and linux-ia64 both use little endian. I don't
> understand why you are barking at us with big endian issue.
>
I can fix this without changing the flags size for those architectures.
1. Introduce *_le_bit() bit operations which takes void *addr
(already I have these functions in the scope of
HAVE_ARCH_EXT2_NON_ATOMIC_BITOPS in my patch)
2. Change flags to __u8 flags[4] or __u8 flags[8] for each architectures.
3. Use *_le_bit() in include/linux/thread_info.h
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-26 3:50 ` Akinobu Mita
@ 2006-01-26 4:12 ` Paul Mackerras
0 siblings, 0 replies; 9+ messages in thread
From: Paul Mackerras @ 2006-01-26 4:12 UTC (permalink / raw)
To: Akinobu Mita
Cc: Chen, Kenneth W, linux-m68k, linux-ia64, ultralinux,
Linux Kernel Development, Andi Kleen, Linux/PPC Development,
'Geert Uytterhoeven',
sparclinux, linux390, linuxsh-dev, parisc-linux
Akinobu Mita writes:
> I can fix this without changing the flags size for those architectures.
>
> 1. Introduce *_le_bit() bit operations which takes void *addr
> (already I have these functions in the scope of
> HAVE_ARCH_EXT2_NON_ATOMIC_BITOPS in my patch)
>
> 2. Change flags to __u8 flags[4] or __u8 flags[8] for each architectures.
>
> 3. Use *_le_bit() in include/linux/thread_info.h
Please don't do this, you'll break the powerpc assembly code that
tests bits in thread_info()->flags.
Paul.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 0/6] RFC: use include/asm-generic/bitops.h
@ 2006-01-25 11:26 Akinobu Mita
2006-01-25 11:34 ` [PATCH 5/6] fix warning on test_ti_thread_flag() Akinobu Mita
0 siblings, 1 reply; 9+ messages in thread
From: Akinobu Mita @ 2006-01-25 11:26 UTC (permalink / raw)
To: linux-kernel
Cc: Richard Henderson, Ivan Kokshaysky, Russell King, Ian Molton,
dev-etrax, David Howells, Yoshinori Sato, Linus Torvalds,
linux-ia64, Hirokazu Takata, linux-m68k, Greg Ungerer,
linux-mips, parisc-linux, linuxppc-dev, linux390, linuxsh-dev,
linuxsh-shmedia-dev, sparclinux, ultralinux, Miles Bader,
Andi Kleen, Chris Zankel
Large number of boilerplate bit operations written in C-language
are scattered around include/asm-*/bitops.h.
These patch series gather them into include/asm-generic/bitops.h. And
- kill duplicated code and comment (about 4000lines)
- use better C-language equivalents
- help porting new architecture (now include/asm-generic/bitops.h is not
referenced from anywhere)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 11:26 [PATCH 0/6] RFC: use include/asm-generic/bitops.h Akinobu Mita
@ 2006-01-25 11:34 ` Akinobu Mita
2006-01-25 12:28 ` Geert Uytterhoeven
2006-01-25 22:28 ` Paul Mackerras
0 siblings, 2 replies; 9+ messages in thread
From: Akinobu Mita @ 2006-01-25 11:34 UTC (permalink / raw)
To: linux-kernel
Cc: Richard Henderson, Ivan Kokshaysky, Russell King, Ian Molton,
dev-etrax, David Howells, Yoshinori Sato, Linus Torvalds,
linux-ia64, Hirokazu Takata, linux-m68k, Greg Ungerer,
linux-mips, parisc-linux, linuxppc-dev, linux390, linuxsh-dev,
linuxsh-shmedia-dev, sparclinux, ultralinux, Miles Bader,
Andi Kleen, Chris Zankel
If the arechitecture is
- BITS_PER_LONG == 64
- struct thread_info.flag 32 is bits
- second argument of test_bit() was void *
Then compiler print error message on test_ti_thread_flags()
in include/linux/thread_info.h
Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
---
thread_info.h | 2 +-
1 files changed, 1 insertion(+), 1 deletion(-)
Index: 2.6-git/include/linux/thread_info.h
===================================================================
--- 2.6-git.orig/include/linux/thread_info.h 2006-01-25 19:07:12.000000000 +0900
+++ 2.6-git/include/linux/thread_info.h 2006-01-25 19:14:26.000000000 +0900
@@ -49,7 +49,7 @@
static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
{
- return test_bit(flag,&ti->flags);
+ return test_bit(flag, (void *)&ti->flags);
}
#define set_thread_flag(flag) \
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 11:34 ` [PATCH 5/6] fix warning on test_ti_thread_flag() Akinobu Mita
@ 2006-01-25 12:28 ` Geert Uytterhoeven
2006-01-25 22:28 ` Paul Mackerras
1 sibling, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2006-01-25 12:28 UTC (permalink / raw)
To: Akinobu Mita
Cc: Linux Kernel Development, Richard Henderson, Ivan Kokshaysky,
Russell King, Ian Molton, dev-etrax, David Howells,
Yoshinori Sato, Linus Torvalds, linux-ia64, Hirokazu Takata,
linux-m68k, Greg Ungerer, Linux/MIPS Development, parisc-linux,
Linux/PPC Development, linux390, linuxsh-dev,
linuxsh-shmedia-dev, sparclinux, ultralinux, Miles Bader,
Andi Kleen, Chris Zankel
On Wed, 25 Jan 2006, Akinobu Mita wrote:
> If the arechitecture is
> - BITS_PER_LONG == 64
> - struct thread_info.flag 32 is bits
> - second argument of test_bit() was void *
>
> Then compiler print error message on test_ti_thread_flags()
> in include/linux/thread_info.h
>
> Signed-off-by: Akinobu Mita <mita@miraclelinux.com>
> ---
> thread_info.h | 2 +-
> 1 files changed, 1 insertion(+), 1 deletion(-)
>
> Index: 2.6-git/include/linux/thread_info.h
> ===================================================================
> --- 2.6-git.orig/include/linux/thread_info.h 2006-01-25 19:07:12.000000000 +0900
> +++ 2.6-git/include/linux/thread_info.h 2006-01-25 19:14:26.000000000 +0900
> @@ -49,7 +49,7 @@
>
> static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
> {
> - return test_bit(flag,&ti->flags);
> + return test_bit(flag, (void *)&ti->flags);
> }
This is not safe. The bitops are defined to work on unsigned long only, so
flags should be changed to unsigned long instead, or you should use a
temporary.
Affected platforms:
- alpha: flags is unsigned int
- ia64, sh, x86_64: flags is __u32
The only affected 64-platforms are little endian, so it will silently work
after your change, though...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 11:34 ` [PATCH 5/6] fix warning on test_ti_thread_flag() Akinobu Mita
2006-01-25 12:28 ` Geert Uytterhoeven
@ 2006-01-25 22:28 ` Paul Mackerras
2006-01-26 0:04 ` David S. Miller
1 sibling, 1 reply; 9+ messages in thread
From: Paul Mackerras @ 2006-01-25 22:28 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-kernel, linux-mips, linux-ia64, Ian Molton, David Howells,
linuxppc-dev, Greg Ungerer, sparclinux, Miles Bader,
Linus Torvalds, Yoshinori Sato, Hirokazu Takata,
linuxsh-shmedia-dev, linux-m68k, Ivan Kokshaysky,
Richard Henderson, Chris Zankel, dev-etrax, ultralinux,
Andi Kleen, linuxsh-dev, linux390, Russell King, parisc-linux
Akinobu Mita writes:
> If the arechitecture is
> - BITS_PER_LONG == 64
> - struct thread_info.flag 32 is bits
> - second argument of test_bit() was void *
>
> Then compiler print error message on test_ti_thread_flags()
> in include/linux/thread_info.h
And correctly so. The correct fix is to make thread_info.flag an
unsigned long. This patch is NAKed.
Paul.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 5/6] fix warning on test_ti_thread_flag()
2006-01-25 22:28 ` Paul Mackerras
@ 2006-01-26 0:04 ` David S. Miller
0 siblings, 0 replies; 9+ messages in thread
From: David S. Miller @ 2006-01-26 0:04 UTC (permalink / raw)
To: paulus
Cc: mita, linux-kernel, linux-mips, linux-ia64, spyro, dhowells,
linuxppc-dev, gerg, sparclinux, uclinux-v850, torvalds, ysato,
takata, linuxsh-shmedia-dev, linux-m68k, ink, rth, chris,
dev-etrax, ultralinux, ak, linuxsh-dev, linux390, rmk,
parisc-linux
From: Paul Mackerras <paulus@samba.org>
Date: Thu, 26 Jan 2006 09:28:02 +1100
> Akinobu Mita writes:
>
> > If the arechitecture is
> > - BITS_PER_LONG == 64
> > - struct thread_info.flag 32 is bits
> > - second argument of test_bit() was void *
> >
> > Then compiler print error message on test_ti_thread_flags()
> > in include/linux/thread_info.h
>
> And correctly so. The correct fix is to make thread_info.flag an
> unsigned long. This patch is NAKed.
I agree.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-01-26 4:12 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-01-25 17:08 [PATCH 5/6] fix warning on test_ti_thread_flag() Chen, Kenneth W
2006-01-25 17:19 ` Geert Uytterhoeven
2006-01-25 20:02 ` Chen, Kenneth W
2006-01-26 3:50 ` Akinobu Mita
2006-01-26 4:12 ` Paul Mackerras
-- strict thread matches above, loose matches on Subject: below --
2006-01-25 11:26 [PATCH 0/6] RFC: use include/asm-generic/bitops.h Akinobu Mita
2006-01-25 11:34 ` [PATCH 5/6] fix warning on test_ti_thread_flag() Akinobu Mita
2006-01-25 12:28 ` Geert Uytterhoeven
2006-01-25 22:28 ` Paul Mackerras
2006-01-26 0:04 ` David S. Miller
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).