RE: [PATCH 5/6] fix warning on test_ti_thread_flag()

RE: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Chen, Kenneth W]

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

RE: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Geert Uytterhoeven]

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

RE: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Chen, Kenneth W]

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.

Re: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Akinobu Mita]

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

Re: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Paul Mackerras]

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.

Re: [PATCH 5/6] fix warning on test_ti_thread_flag()

[David S. Miller]

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.

Re: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Paul Mackerras]

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.

Re: [PATCH 5/6] fix warning on test_ti_thread_flag()

[Geert Uytterhoeven]

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

[PATCH 5/6] fix warning on test_ti_thread_flag()

[Akinobu Mita]

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) \