[resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Jesper Juhl]

I send in the patch below a while back but never recieved any response.
Now I'm resending it in the hope that it might be added to -mm.
The patch still applies cleanly to 2.6.12-rc6-mm1

-- 
Jesper Juhl


---------- Forwarded message ----------
Date: Fri, 18 Mar 2005 00:43:33 +0100 (CET)
From: Jesper Juhl <juhl-lkml@dif.dk>
To: linux-kernel <linux-kernel@vger.kernel.org>
Cc: Walt Drummond <drummond@valinux.com>,
    David Mosberger-Tang <davidm@hpl.hp.com>,
    Stephane Eranian <eranian@hpl.hp.com>
Subject: [PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()


This little function in include/linux/efi.h :

static inline int efi_range_is_wc(unsigned long start, unsigned long len)
{
        int i;

        for (i = 0; i < len; i += (1UL << EFI_PAGE_SHIFT)) {
                unsigned long paddr = __pa(start + i);
                if (!(efi_mem_attributes(paddr) & EFI_MEMORY_WC))
                        return 0;
        }
        /* The range checked out */
        return 1;
}

generates this warning when building with gcc -W : 

include/linux/efi.h: In function `efi_range_is_wc':
include/linux/efi.h:320: warning: comparison between signed and unsigned

It looks to me like a significantly large 'len' passed in could cause the 
loop to never end. Isn't it safer to make 'i' an unsigned long as well? 
Like this little patch below (which of course also kills the warning) :


Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -up linux-2.6.11-mm4-orig/include/linux/efi.h linux-2.6.11-mm4/include/linux/efi.h
--- linux-2.6.11-mm4-orig/include/linux/efi.h	2005-03-16 15:45:35.000000000 +0100
+++ linux-2.6.11-mm4/include/linux/efi.h	2005-03-18 00:34:36.000000000 +0100
@@ -315,7 +315,7 @@ extern struct efi_memory_map memmap;
  */
 static inline int efi_range_is_wc(unsigned long start, unsigned long len)
 {
-	int i;
+	unsigned long i;
 
 	for (i = 0; i < len; i += (1UL << EFI_PAGE_SHIFT)) {
 		unsigned long paddr = __pa(start + i);

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Andrew Morton]

Jesper Juhl <juhl-lkml@dif.dk> wrote:
>
> I send in the patch below a while back but never recieved any response.
> Now I'm resending it in the hope that it might be added to -mm.

There are surely many warnings in the tree, hence I'm not really interested
in patches which only fix `gcc -W' warnings.

How many are there?

> It looks to me like a significantly large 'len' passed in could cause the 
> loop to never end. Isn't it safer to make 'i' an unsigned long as well? 

Nope.  All operations which mix signed and unsigned types promote the
signed type to unsigned.

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Jesper Juhl]

On Thu, 16 Jun 2005, Andrew Morton wrote:

> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> >
> > I send in the patch below a while back but never recieved any response.
> > Now I'm resending it in the hope that it might be added to -mm.
> 
> There are surely many warnings in the tree, hence I'm not really interested
> in patches which only fix `gcc -W' warnings.
> 

Ok, in that case I won't bother you directly with such patches any more 
but instead let them trickle into maintainers trees when they will take 
them.

And yes, I know it's very trivial stuff and it doesn't make much of a 
difference to the "big picture", but my attitude towards that is that no 
issue is too small to be addressed, and since I'm not able to adress many 
of the larger issues I try to address the smaller ones that I'm able to 
handle, and when I run out of those I start nitpicking with the really 
trivial stuff (like gcc -W warnings) - all with the purpose of helping our 
kernel be the very best it can, even if my contribution might be very 
minor in some cases.


> How many are there?
> 

With the .config I use here a regular build gives me 10 warnings. A build 
with gcc -W of the same config gives me 100177 warnings.


> > It looks to me like a significantly large 'len' passed in could cause the 
> > loop to never end. Isn't it safer to make 'i' an unsigned long as well? 
> 
> Nope.  All operations which mix signed and unsigned types promote the
> signed type to unsigned.
> 
Hmm, right, then the only bennefit of the patch as-is is to silence the 
gcc -W warning. But since it can be done 100% safe and the change to use 
an unsigned value for the counter is (at least to me) the logical and 
obviously correct thing to do, I still think the patch has merrit as a 
purely "for pedantic correctness" fix.


-- 
Jesper

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Arnd Bergmann]

On Dunnersdag 16 Juni 2005 23:02, Jesper Juhl wrote:
> On Thu, 16 Jun 2005, Andrew Morton wrote:
> > There are surely many warnings in the tree, hence I'm not really interested
> > in patches which only fix `gcc -W' warnings.
> > 
> 
> Ok, in that case I won't bother you directly with such patches any more 
> but instead let them trickle into maintainers trees when they will take 
> them.
> 
> And yes, I know it's very trivial stuff and it doesn't make much of a 
> difference to the "big picture", but my attitude towards that is that no 
> issue is too small to be addressed, and since I'm not able to adress many 
> of the larger issues I try to address the smaller ones that I'm able to 
> handle, and when I run out of those I start nitpicking with the really 
> trivial stuff (like gcc -W warnings) - all with the purpose of helping our 
> kernel be the very best it can, even if my contribution might be very 
> minor in some cases.

I have a patch that optionally enables some of the interesting warnings
that gcc supports (e.g. -Wmissing-format-attribute -Wmissing-declarations
-Wundef -Wwrite-strings).

It has four different levels:

- quiet (current warnings minus -Wdeprecated-declarations)
- normal (some interesting ones added that are not too noisy)
- more (all interesting ones, including some noisier ones like 
  -Wmissing-declarations)
- overkill (-W and some more that only make sense for statistic
  analysis)

I have the base patch and some more patches that fix the most annoying 
warnings. I find them more useful than the signed vs unsigned comparison
fixes you are doing right now, but don't have the time to split my
patches up into obvious chunks.

Jesper, are you interested in my stuff and willing to continue that work?
I'd suggest to fix the warnings at 'normal' level first and then
integrate the patch for configurable warning levels into -mm.

	Arnd <><

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Jesper Juhl]

On Thu, 16 Jun 2005, Arnd Bergmann wrote:

> On Dunnersdag 16 Juni 2005 23:02, Jesper Juhl wrote:
> > On Thu, 16 Jun 2005, Andrew Morton wrote:
> > > There are surely many warnings in the tree, hence I'm not really interested
> > > in patches which only fix `gcc -W' warnings.
> > > 
> > 
> > Ok, in that case I won't bother you directly with such patches any more 
> > but instead let them trickle into maintainers trees when they will take 
> > them.
> > 
> > And yes, I know it's very trivial stuff and it doesn't make much of a 
> > difference to the "big picture", but my attitude towards that is that no 
> > issue is too small to be addressed, and since I'm not able to adress many 
> > of the larger issues I try to address the smaller ones that I'm able to 
> > handle, and when I run out of those I start nitpicking with the really 
> > trivial stuff (like gcc -W warnings) - all with the purpose of helping our 
> > kernel be the very best it can, even if my contribution might be very 
> > minor in some cases.
> 
> I have a patch that optionally enables some of the interesting warnings
> that gcc supports (e.g. -Wmissing-format-attribute -Wmissing-declarations
> -Wundef -Wwrite-strings).
> 
> It has four different levels:
> 
> - quiet (current warnings minus -Wdeprecated-declarations)
> - normal (some interesting ones added that are not too noisy)
> - more (all interesting ones, including some noisier ones like 
>   -Wmissing-declarations)
> - overkill (-W and some more that only make sense for statistic
>   analysis)
> 
> I have the base patch and some more patches that fix the most annoying 
> warnings. I find them more useful than the signed vs unsigned comparison
> fixes you are doing right now, but don't have the time to split my
> patches up into obvious chunks.
> 
> Jesper, are you interested in my stuff

Certainly.


> and willing to continue that work?

To the best of my abilities, yes. I'd like to take a look at those 
patches. 
If nothing else it sounds like a good way for me to cut my current 
(extra warning enabled) build logs down to size and focus on the more 
relevant of the issues. And perhaps, if I can find the time for it, I can 
split the other patches you have into some sane chunks and start 
submitting them to the relevant maintainers.


> I'd suggest to fix the warnings at 'normal' level first and then
> integrate the patch for configurable warning levels into -mm.
> 
Sounds like a resonable plan to me.  Send your stuff along and I'll put 
some time into it when and where I can find it :-)


--
Jesper 

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Richard B. Johnson]

Well long and int are the same size on ix86. What you really need
is 'size_t' for counters. That's the largest unsigned integer that
will fit in a register on the target platform. It is platform-
specific, therefore defined in stddef.h. No index or return-value
is supposed to be larger than what can be represented in size_t,
therefore it is a good type to use.

Note that on 64-bit platforms, size_t will be larger than an unsigned
int. This is good.

On Thu, 16 Jun 2005, Jesper Juhl wrote:

> I send in the patch below a while back but never recieved any response.
> Now I'm resending it in the hope that it might be added to -mm.
> The patch still applies cleanly to 2.6.12-rc6-mm1
>
> -- 
> Jesper Juhl
>
>
> ---------- Forwarded message ----------
> Date: Fri, 18 Mar 2005 00:43:33 +0100 (CET)
> From: Jesper Juhl <juhl-lkml@dif.dk>
> To: linux-kernel <linux-kernel@vger.kernel.org>
> Cc: Walt Drummond <drummond@valinux.com>,
>    David Mosberger-Tang <davidm@hpl.hp.com>,
>    Stephane Eranian <eranian@hpl.hp.com>
> Subject: [PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()
>
>
> This little function in include/linux/efi.h :
>
> static inline int efi_range_is_wc(unsigned long start, unsigned long len)
> {
>        int i;
>
>        for (i = 0; i < len; i += (1UL << EFI_PAGE_SHIFT)) {
>                unsigned long paddr = __pa(start + i);
>                if (!(efi_mem_attributes(paddr) & EFI_MEMORY_WC))
>                        return 0;
>        }
>        /* The range checked out */
>        return 1;
> }
>
> generates this warning when building with gcc -W :
>
> include/linux/efi.h: In function `efi_range_is_wc':
> include/linux/efi.h:320: warning: comparison between signed and unsigned
>
> It looks to me like a significantly large 'len' passed in could cause the
> loop to never end. Isn't it safer to make 'i' an unsigned long as well?
> Like this little patch below (which of course also kills the warning) :
>
>
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
>
> diff -up linux-2.6.11-mm4-orig/include/linux/efi.h linux-2.6.11-mm4/include/linux/efi.h
> --- linux-2.6.11-mm4-orig/include/linux/efi.h	2005-03-16 15:45:35.000000000 +0100
> +++ linux-2.6.11-mm4/include/linux/efi.h	2005-03-18 00:34:36.000000000 +0100
> @@ -315,7 +315,7 @@ extern struct efi_memory_map memmap;
>  */
> static inline int efi_range_is_wc(unsigned long start, unsigned long len)
> {
> -	int i;
> +	unsigned long i;
>
> 	for (i = 0; i < len; i += (1UL << EFI_PAGE_SHIFT)) {
> 		unsigned long paddr = __pa(start + i);
>
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.11.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Jesper Juhl]

On Thu, 16 Jun 2005, Richard B. Johnson wrote:

> 
> Well long and int are the same size on ix86. What you really need
> is 'size_t' for counters. That's the largest unsigned integer that
> will fit in a register on the target platform. It is platform-
> specific, therefore defined in stddef.h. No index or return-value
> is supposed to be larger than what can be represented in size_t,
> therefore it is a good type to use.
> 
> Note that on 64-bit platforms, size_t will be larger than an unsigned
> int. This is good.
> 
Good info, thanks Richard.

-- 
Jesper

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Arnd Bergmann]

On Dunnersdag 16 Juni 2005 23:07, Jesper Juhl wrote:
> > 
> > Well long and int are the same size on ix86. What you really need
> > is 'size_t' for counters. That's the largest unsigned integer that
> > will fit in a register on the target platform. It is platform-
> > specific, therefore defined in stddef.h. No index or return-value
> > is supposed to be larger than what can be represented in size_t,
> > therefore it is a good type to use.
> > 
> > Note that on 64-bit platforms, size_t will be larger than an unsigned
> > int. This is good.
> > 
> Good info, thanks Richard.

Actually not that good. size_t is meant to represent the size of a structure
or array in units of characters. It also happens to be the same size as an
integer register on all architectures that Linux supports, just like unsigned
long.

The most common convention in the kernel is to use 'unsigned long' for
every integer that needs to be as large as possible without the overhead
of strict 64 bit types (like here), and also for integer values that need
to be the same size as a pointer.

OTOH size_t should be used only to count bytes, like in standard C usage.

	Arnd <><

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[David Mosberger]

>>>>> On Thu, 16 Jun 2005 22:21:50 +0200 (CEST), Jesper Juhl <juhl-lkml@dif.dk> said:

  Jesper> I send in the patch below a while back but never recieved
  Jesper> any response.  Now I'm resending it in the hope that it
  Jesper> might be added to -mm.  The patch still applies cleanly to
  Jesper> 2.6.12-rc6-mm1

The patch looks fine to me.

	--david

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Jesse Barnes]

On Thursday, June 16, 2005 2:01 pm, David Mosberger wrote:
> >>>>> On Thu, 16 Jun 2005 22:21:50 +0200 (CEST), Jesper Juhl
> >>>>> <juhl-lkml@dif.dk> said:
>
>   Jesper> I send in the patch below a while back but never recieved
>   Jesper> any response.  Now I'm resending it in the hope that it
>   Jesper> might be added to -mm.  The patch still applies cleanly to
>   Jesper> 2.6.12-rc6-mm1
>
> The patch looks fine to me.

Yep, looks good.  I was the one that introduced this bug.  It's hard to 
hit, but we should still fix it.  Thanks Jesper.

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Jesse

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Arnd Bergmann <arnd@arndb.de>]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

Jesper wrote:

>> Jesper, are you interested in my stuff
>
>Certainly.
>

Ok, here is the warning level patch for reference. I'm sending the rest of my stuff
to you off-list since it is rather largish and I don't have a working mail client
on this machine.

      Arnd <><

[-- Attachment #2: warn-levels.diff --]
[-- Type: application/octet-stream, Size: 5116 bytes --]

[PATCH] kbuild: selectable warning levels

This introduces four different levels of compiler warnings to the
kernel build environment. The idea is to enable developers to get the
extra static code checks that newer gcc versions provide without
annoying users with false positives.

I have tested this with gcc versions 2.95 through 4.0 on i386 as
well as some cross builds for x86_64, ppc64 and s390. The conditional
warning settings are needed to get some of the options that pre-4.0
compilers don't support.

I have not tested this at all with non-GNU compilers, so it
probably is not ready for inclusion in -mm at this point.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Index: linus-2.6/Makefile
===================================================================
--- linus-2.6.orig/Makefile	2005-06-17 00:57:11.000000000 +0200
+++ linus-2.6/Makefile	2005-06-17 01:54:36.000000000 +0200
@@ -348,8 +348,7 @@
 
 CPPFLAGS        := -D__KERNEL__ $(LINUXINCLUDE)
 
-CFLAGS 		:= -Wall -Wstrict-prototypes -Wno-trigraphs \
-	  	   -fno-strict-aliasing -fno-common \
+CFLAGS 		:= -fno-strict-aliasing -fno-common \
 		   -ffreestanding
 AFLAGS		:= -D__ASSEMBLY__
 
@@ -533,11 +532,46 @@
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 CHECKFLAGS     += $(NOSTDINC_FLAGS)
 
+# standard warnings
+CWARN_LESS         += -Wall -Wstrict-prototypes -Wno-trigraphs
 # warn about C99 declaration after statement
-CFLAGS += $(call cc-option,-Wdeclaration-after-statement,)
+CWARN_LESS         += $(call cc-option,-Wdeclaration-after-statement,)
+CWARN_LESS_COND    +=
+ifdef CONFIG_WARN_ERROR
+CWARN_LESS         += -Werror
+endif
+ifdef CONFIG_WARN_LESS
+CWARN_LESS         += -Wno-deprecated-declarations
+CWARN_LESS_COND    += -Wno-pointer-sign
+CFLAGS             += $(CWARN_LESS) $(call cc-option,$(CWARN_LESS_COND),)
+endif
+
+# warn about common problems
+CWARN_NORMAL       += $(CWARN_LESS) -Winline -Wcast-align -Wundef -Wformat
+CWARN_NORMAL       += -Wmissing-noreturn
+CWARN_NORMAL_COND  += $(CWARN_LESS_COND) -Wdisabled-optimization
+CWARN_NORMAL_COND  += -Wmissing-format-attribute -Wendif-labels
+ifdef CONFIG_WARN_NORMAL
+CWARN_NORMAL_COND  += -Wno-pointer-sign
+CFLAGS             += $(CWARN_NORMAL) $(call cc-option,$(CWARN_NORMAL_COND),)
+endif
 
-# disable pointer signedness warnings in gcc 4.0
-CFLAGS += $(call cc-option,-Wno-pointer-sign,)
+# these can get rather noisy
+CWARN_MORE         += $(CWARN_NORMAL) -Wredundant-decls -Wnested-externs
+CWARN_MORE         += -Wmissing-prototypes -Wwrite-strings -Wshadow
+CWARN_MORE_COND    += $(CWARN_NORMAL_COND)
+ifdef CONFIG_WARN_MORE
+CFLAGS             += $(CWARN_MORE) $(call cc-option,$(CWARN_MORE_COND),)
+endif
+
+# these usually don't do any good
+CWARN_TOOMUCH      += $(CWARN_MORE) -Waggregate-return -Wsign-compare
+CWARN_TOOMUCH      += -W -Wlarger-than-8192 -Wconversion -Wbad-function-cast
+CWARN_TOOMUCH      += -Wcast-qual
+CWARN_TOOMUCH_COND += $(CWARN_MORE_COND) -Wpadded -Wpacked -Wunreachable-code
+ifdef CONFIG_WARN_TOOMUCH
+CFLAGS             += $(CWARN_TOOMUCH) $(call cc-option,$(CWARN_TOOMUCH_COND),)
+endif
 
 # Default kernel image to build when no specific target is given.
 # KBUILD_IMAGE may be overruled on the commandline or
Index: linus-2.6/lib/Kconfig.debug
===================================================================
--- linus-2.6.orig/lib/Kconfig.debug	2005-06-17 00:57:11.000000000 +0200
+++ linus-2.6/lib/Kconfig.debug	2005-06-17 01:52:22.000000000 +0200
@@ -159,3 +159,49 @@
 	  If you don't debug the kernel, you can say N, but we may not be able
 	  to solve problems without frame pointers.
 
+choice
+	prompt "Compiler warning level"
+	default WARN_NORMAL
+	help
+	  Select how much the compiler should warn about kernel code.
+	  If unsure, select "Normal".
+
+config WARN_LESS
+	bool "Minimal"
+	help
+	  In minimal warning mode, only the traditional -Wall warnings are
+	  enabled, with the exception of deprecated interfaces. For common
+	  configurations, there ought to be no warnings at this level.
+	  Select this if you also want to built with -Werror.
+
+config WARN_NORMAL
+	bool "Normal"
+	help
+	  The normal warning level will warn about some common problems that
+	  may need to be fixed up. If you are maintaining code that is
+	  warned about at this level, you should try to fix that.
+
+config WARN_MORE
+	bool "More"
+	help
+	  Select "More" to enable some warnings that are rather noisy for
+	  existing parts of the kernel. This is most useful to check new code
+	  before it is submitted for inclusion.
+
+config WARN_TOOMUCH
+	bool "All"
+	help
+	  Enabling all warnings will create many warnings about good code that
+	  becomes less readable if you try to work around the warning. Patches
+	  to shut up warnings at this level are likely to be rejected unless
+	  they fix real problems.
+
+endchoice
+
+config WARN_ERROR
+	bool "Make all build warnings errors"
+	depends on WARN_LESS
+	help
+	  When this option is enabled, all compiler warnings are turned into
+	  errors, making it impossible to build the kernel while some warnings
+	  are left.

Re: [resend][PATCH] avoid signed vs unsigned comparison in efi_range_is_wc()

[Jesper Juhl]

On 6/17/05, Arnd Bergmann <arnd@arndb.de> <arndb@onlinehome.de> wrote:
> Jesper wrote:
> 
> >> Jesper, are you interested in my stuff
> >
> >Certainly.
> >
> 
> Ok, here is the warning level patch for reference. I'm sending the rest of my stuff
> to you off-list since it is rather largish and I don't have a working mail client
> on this machine.
> 

Thank you. This looks useful.

I also recieved your other patches and I'll see what I can do to
double-check them, make sure they apply to current kernels and then
start submitting them to the relevant people.

Thanks.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html