[PATCH][MIPS] fix run_uncached warning about 32bit kernel

[PATCH][MIPS] fix run_uncached warning about 32bit kernel

[Yoichi Yuasa]

Hi Ralf,

This patch has fixed warning about 32 bit kernel.

arch/mips/lib/uncached.c: In function 'run_uncached':
arch/mips/lib/uncached.c:47: warning: comparison is always true due to limited range of data type
arch/mips/lib/uncached.c:48: warning: comparison is always false due to limited range of data type
arch/mips/lib/uncached.c:57: warning: comparison is always true due to limited range of data type
arch/mips/lib/uncached.c:58: warning: comparison is always false due to limited range of data type

Yoichi

Signed-off-by: Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>

diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/lib/uncached.c mips/arch/mips/lib/uncached.c
--- mips-orig/arch/mips/lib/uncached.c	2006-11-13 19:32:04.002117500 +0900
+++ mips/arch/mips/lib/uncached.c	2006-11-13 19:29:07.087061000 +0900
@@ -44,20 +44,24 @@ unsigned long __init run_uncached(void *
 
 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
+#ifdef CONFIG_64BIT
 	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
 		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
 		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
 				     XKPHYS_TO_PHYS((long long)sp));
+#endif
 	else {
 		BUG();
 		usp = sp;
 	}
 	if (lfunc >= (long)CKSEG0 && lfunc < (long)CKSEG2)
 		ufunc = CKSEG1ADDR(lfunc);
+#ifdef CONFIG_64BIT
 	else if ((long long)lfunc >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
 		 (long long)lfunc < (long long)PHYS_TO_XKPHYS(8LL, 0))
 		ufunc = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
 				       XKPHYS_TO_PHYS((long long)lfunc));
+#endif
 	else {
 		BUG();
 		ufunc = lfunc;

Re: [PATCH][MIPS] fix run_uncached warning about 32bit kernel

[Ralf Baechle]

On Tue, Feb 06, 2007 at 10:59:22AM +0900, Yoichi Yuasa wrote:

> arch/mips/lib/uncached.c: In function 'run_uncached':
> arch/mips/lib/uncached.c:47: warning: comparison is always true due to limited range of data type
> arch/mips/lib/uncached.c:48: warning: comparison is always false due to limited range of data type
> arch/mips/lib/uncached.c:57: warning: comparison is always true due to limited range of data type
> arch/mips/lib/uncached.c:58: warning: comparison is always false due to limited range of data type

Thanks, applied.

  Ralf

Re: [PATCH][MIPS] fix run_uncached warning about 32bit kernel

[Maciej W. Rozycki]

On Tue, 6 Feb 2007, Ralf Baechle wrote:

> On Tue, Feb 06, 2007 at 10:59:22AM +0900, Yoichi Yuasa wrote:
> 
> > arch/mips/lib/uncached.c: In function 'run_uncached':
> > arch/mips/lib/uncached.c:47: warning: comparison is always true due to limited range of data type
> > arch/mips/lib/uncached.c:48: warning: comparison is always false due to limited range of data type
> > arch/mips/lib/uncached.c:57: warning: comparison is always true due to limited range of data type
> > arch/mips/lib/uncached.c:58: warning: comparison is always false due to limited range of data type
> 
> Thanks, applied.

 "Fixing" bugs in the compiler, huh? ;-)  I suppose there should be a note 
somewhere nearby then, so there is a remote chance to remove the clutter 
in the future.

  Maciej

Re: [PATCH][MIPS] fix run_uncached warning about 32bit kernel

[Ralf Baechle]

On Tue, Feb 06, 2007 at 06:20:39PM +0000, Maciej W. Rozycki wrote:

> > > arch/mips/lib/uncached.c: In function 'run_uncached':
> > > arch/mips/lib/uncached.c:47: warning: comparison is always true due to limited range of data type
> > > arch/mips/lib/uncached.c:48: warning: comparison is always false due to limited range of data type
> > > arch/mips/lib/uncached.c:57: warning: comparison is always true due to limited range of data type
> > > arch/mips/lib/uncached.c:58: warning: comparison is always false due to limited range of data type
> > 
> > Thanks, applied.
> 
>  "Fixing" bugs in the compiler, huh? ;-)  I suppose there should be a note 
> somewhere nearby then, so there is a remote chance to remove the clutter 
> in the future.

Well, some of the warnings are also simply due to broken code.  This is
the result of preprocessing the code without Yoichi's patch applied:

[...]
 if (sp >= (long)0x80000000 && sp < (long)0xc0000000)
  usp = ((((int)(int)(sp)) & 0x1fffffff) | 0xa0000000);
 else if ((long long)sp >= (long long)(0x8000000000000000LL | ((0LL)<<59) | (0)) &&
   (long long)sp < (long long)(0x8000000000000000LL | ((8LL)<<59) | (0)))
  usp = (0x8000000000000000LL | (((long long)2)<<59) | ((((long long)sp) & -1)));

else {
  do { __asm__ __volatile__("break %0" : : "i" (512)); } while (0);
  usp = sp;
 }
[...]

So (0x8000000000000000LL | ((0LL)<<59) | (0)) is 0x8000000000000000LL which
then is casted to _signed_ long long, so becomes -9223372036854775808, the
most negative representable number so the two "comparison is always true
due to limited range of data type" warnings are perfectly correct.

Treating addresses as signed is a dangerous thing and we reallly only
should do it where extending 32-bit addresses to 64-bit because that's
what the architecture does.  So I would suggest as part of cleaning u the
mess something like below totally untested patch.

Comments?

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/include/asm-mips/addrspace.h b/include/asm-mips/addrspace.h
index c627508..ff2dd38 100644
--- a/include/asm-mips/addrspace.h
+++ b/include/asm-mips/addrspace.h
@@ -10,6 +10,7 @@
 #ifndef _ASM_ADDRSPACE_H
 #define _ASM_ADDRSPACE_H
 
+#include <linux/types.h>
 #include <spaces.h>
 
 /*
@@ -22,12 +23,12 @@
 #define _CONST64_(x)	x
 #else
 #define _ATYPE_		__PTRDIFF_TYPE__
-#define _ATYPE32_	int
-#define _ATYPE64_	__s64
+#define _ATYPE32_	unsigned int
+#define _ATYPE64_	__u64
 #ifdef CONFIG_64BIT
-#define _CONST64_(x)	x ## L
+#define _CONST64_(x)	x ## UL
 #else
-#define _CONST64_(x)	x ## LL
+#define _CONST64_(x)	x ## ULL
 #endif
 #endif
 

Re: [PATCH][MIPS] fix run_uncached warning about 32bit kernel

[Maciej W. Rozycki]

On Thu, 8 Feb 2007, Ralf Baechle wrote:

> Well, some of the warnings are also simply due to broken code.  This is
> the result of preprocessing the code without Yoichi's patch applied:
> 
> [...]
>  if (sp >= (long)0x80000000 && sp < (long)0xc0000000)
>   usp = ((((int)(int)(sp)) & 0x1fffffff) | 0xa0000000);
>  else if ((long long)sp >= (long long)(0x8000000000000000LL | ((0LL)<<59) | (0)) &&
>    (long long)sp < (long long)(0x8000000000000000LL | ((8LL)<<59) | (0)))
>   usp = (0x8000000000000000LL | (((long long)2)<<59) | ((((long long)sp) & -1)));
> 
> else {
>   do { __asm__ __volatile__("break %0" : : "i" (512)); } while (0);
>   usp = sp;
>  }
> [...]
> 
> So (0x8000000000000000LL | ((0LL)<<59) | (0)) is 0x8000000000000000LL which
> then is casted to _signed_ long long, so becomes -9223372036854775808, the
> most negative representable number so the two "comparison is always true
> due to limited range of data type" warnings are perfectly correct.

 They are neither correct nor expected.  And the problem is not 
0x8000000000000000LL being the most negative representable number, but 
"sp" being a variable of a 32-bit type and being compared against the 
constant.  In this case GCC seems to completely ignore the cast to "long 
long" and treats it as if the comparison was done between types of 
different widths.

 Try building this program:

$ cat range.c
int foo(sp_t sp)
{
	if (sp >= (long)0x80000000 && sp < (long)0xc0000000)
		return 0;
	else if ((long long)sp >= (long long)0x8000000000000000LL &&
		 (long long)sp < (long long)0xc000000000000000LL)
		return 1;
	else
		return 2;
}
$ mips64el-linux-gcc -O2 -Wall -Dsp_t=long -c range.c
$ mips64el-linux-gcc -O2 -Wall -Dsp_t=int -c range.c
range.c: In function 'foo':
range.c:3: warning: comparison is always false due to limited range of data type
range.c:3: warning: comparison is always true due to limited range of data type
range.c:5: warning: comparison is always true due to limited range of data type
range.c:6: warning: comparison is always false due to limited range of data type
$

Notice how GCC complains about both 0x8000000000000000LL and 
0xc000000000000000LL.

 I think there was a bug report associated with it -- let me see...  Yep: 
"http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12963".

> Treating addresses as signed is a dangerous thing and we reallly only
> should do it where extending 32-bit addresses to 64-bit because that's
> what the architecture does.  So I would suggest as part of cleaning u the
> mess something like below totally untested patch.

 I have used signed types here on purpose not to cross to the user segment 
(positive range) accidentally.  But I do not insist on keeping them if 
they were to hurt somebody's eyes.  Your change is not going to fix the 
problem anyway -- if I change the condition in the program above to:

	else if ((unsigned long long)sp >= (unsigned long long)0x8000000000000000ULL &&
		 (unsigned long long)sp < (unsigned long long)0xc000000000000000ULL)

then unfortunately the warnings persist (I am pretty sure I did this kind 
of testing before committing these bits, to make sure the warning was 
unavoidable).

  Maciej