compile time warnings

compile time warnings

[Ingo Brueckl]

Maybe somebody noticed already, with kernel 2.6.28 and gcc 4.3.2 there are a
few compile time warnings:

  arch/x86/kernel/setup.c:742: warning: 'dmi_low_memory_corruption' defined but not used

  arch/x86/mm/init_32.c: In function 'pagetable_init':
  arch/x86/mm/init_32.c:515: warning: unused variable 'pgd_base'

  drivers/acpi/tables/tbfadt.c: In function 'acpi_tb_create_local_fadt':
  /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds

  drivers/usb/core/hcd.c: In function 'usb_hcd_poll_rh_status':
  /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds

  WARNING: modpost: Found 1 section mismatch(es).
  To see full details build your kernel with:
  'make CONFIG_DEBUG_SECTION_MISMATCH=y'

Re: compile time warnings

[Jesper Juhl]

On Thu, 1 Jan 2009, Ingo Brueckl wrote:

> Maybe somebody noticed already, with kernel 2.6.28 and gcc 4.3.2 there are a
> few compile time warnings:
> 
I've looked at these with the kernel from git as of today. I've only read 
the code, not build it.

>   arch/x86/kernel/setup.c:742: warning: 'dmi_low_memory_corruption' defined but not used
> 
I see dmi_low_memory_corruption() as well as usage of it both inside 
#ifdef CONFIG_X86_RESERVE_LOW_64K - looks fine to me:

#ifdef CONFIG_X86_RESERVE_LOW_64K
static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
{
...
static struct dmi_system_id __initdata bad_bios_dmi_table[] = {
#ifdef CONFIG_X86_RESERVE_LOW_64K
        {
                .callback = dmi_low_memory_corruption,
...


>   arch/x86/mm/init_32.c: In function 'pagetable_init':
>   arch/x86/mm/init_32.c:515: warning: unused variable 'pgd_base'
> 

I see this : 

static void __init pagetable_init(void)
{
        pgd_t *pgd_base = swapper_pg_dir;

        permanent_kmaps_init(pgd_base);
}

pgd_base is very much used...

>   drivers/acpi/tables/tbfadt.c: In function 'acpi_tb_create_local_fadt':
>   /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds
> 
>   drivers/usb/core/hcd.c: In function 'usb_hcd_poll_rh_status':
>   /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds
> 
Not sure about these.

-- 
Jesper Juhl <jj@chaosbits.net>
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

Re: compile time warnings

[Tom Spink]

2009/1/1 Jesper Juhl <jj@chaosbits.net>:
> On Thu, 1 Jan 2009, Ingo Brueckl wrote:
[snip]

Hi,

> pgd_base is very much used...

It's probably something to do with:

# define permanent_kmaps_init(pgd_base)         do { } while (0)

Which is within the #else part of #if CONFIG_HIGHMEM.  So, if
CONFIG_HIGHMEM is not set, permanent_kmaps_init gets wiped out, and
therefore that warning will be issued.

Perhaps changing that to an empty inline would remove the warning?

-- 
Tom Spink
G. Gordon Liddy  - "Obviously crime pays, or there'd be no crime."

Re: compile time warnings

[Harvey Harrison]

On Fri, 2009-01-02 at 01:35 +0000, Tom Spink wrote:
> 2009/1/1 Jesper Juhl <jj@chaosbits.net>:
> > On Thu, 1 Jan 2009, Ingo Brueckl wrote:
> [snip]
> 
> Hi,
> 
> > pgd_base is very much used...
> 
> It's probably something to do with:
> 
> # define permanent_kmaps_init(pgd_base)         do { } while (0)
> 
> Which is within the #else part of #if CONFIG_HIGHMEM.  So, if
> CONFIG_HIGHMEM is not set, permanent_kmaps_init gets wiped out, and
> therefore that warning will be issued.
> 
> Perhaps changing that to an empty inline would remove the warning?
> 

If it can be an inline, it should be, otherwise:

# define permanent_kmaps_init(pgd_base) do { (void)(pgd_base) } while (0)

would do the trick.

Harvey

Re: compile time warnings

[Jesper Juhl]

On Fri, 2 Jan 2009, Tom Spink wrote:

> 2009/1/1 Jesper Juhl <jj@chaosbits.net>:
> > On Thu, 1 Jan 2009, Ingo Brueckl wrote:
> [snip]
> 
> Hi,
> 
> > pgd_base is very much used...
> 
> It's probably something to do with:
> 
> # define permanent_kmaps_init(pgd_base)         do { } while (0)
> 
> Which is within the #else part of #if CONFIG_HIGHMEM.  So, if
> CONFIG_HIGHMEM is not set, permanent_kmaps_init gets wiped out, and
> therefore that warning will be issued.
> 
> Perhaps changing that to an empty inline would remove the warning?
> 
Yeah, I noticed that as well after sending the mail.
Another way to silence the warning (which I think is nicer) would be 
something like this;


Silence 'unused variable' warning in arch/x86/mm/init_32.c::pagetable_init

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 8655b5b..0affa8e 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -511,9 +511,7 @@ static void __init early_ioremap_page_table_range_init(pgd_t *pgd_base)
 
 static void __init pagetable_init(void)
 {
-	pgd_t *pgd_base = swapper_pg_dir;
-
-	permanent_kmaps_init(pgd_base);
+	permanent_kmaps_init((pgd_t *)swapper_pg_dir);
 }
 
 #ifdef CONFIG_ACPI_SLEEP
 

-- 
Jesper Juhl <jj@chaosbits.net>
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

Re: compile time warnings

[Ingo Molnar]

* Jesper Juhl <jj@chaosbits.net> wrote:

> On Fri, 2 Jan 2009, Tom Spink wrote:
> 
> > 2009/1/1 Jesper Juhl <jj@chaosbits.net>:
> > > On Thu, 1 Jan 2009, Ingo Brueckl wrote:
> > [snip]
> > 
> > Hi,
> > 
> > > pgd_base is very much used...
> > 
> > It's probably something to do with:
> > 
> > # define permanent_kmaps_init(pgd_base)         do { } while (0)
> > 
> > Which is within the #else part of #if CONFIG_HIGHMEM.  So, if
> > CONFIG_HIGHMEM is not set, permanent_kmaps_init gets wiped out, and
> > therefore that warning will be issued.
> > 
> > Perhaps changing that to an empty inline would remove the warning?
> > 
> Yeah, I noticed that as well after sending the mail.
> Another way to silence the warning (which I think is nicer) would be 
> something like this;
> 
> 
> Silence 'unused variable' warning in arch/x86/mm/init_32.c::pagetable_init
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
> 
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 8655b5b..0affa8e 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -511,9 +511,7 @@ static void __init early_ioremap_page_table_range_init(pgd_t *pgd_base)
>  
>  static void __init pagetable_init(void)
>  {
> -	pgd_t *pgd_base = swapper_pg_dir;
> -
> -	permanent_kmaps_init(pgd_base);
> +	permanent_kmaps_init((pgd_t *)swapper_pg_dir);
>  }

no, this only works around the warning by 'silencing' it (it also includes 
an ugly type cast) - instead of fixing the core problem.

The core problem is that permanent_kmaps_init() is a CPP macro in the 
!HIGHMEM case - so the right fix would be to convert that to a proper C 
inline function. (same for set_highmem_pages_init() while at it)

Would you mind to send a patch for that that we could push to Linus?

<soapbox>

The highest quality fixes that are motivated by compiler warnings are the 
ones that do not actually 'fix a warning', but instead improve/reshape 
some code so that as a side-effect the warning goes away.

If you ever see a patch that 'silences a warning', it usually shows that 
the deeper problem has not been fully understood. (Except of course if the 
warning shows a genuine bug in the code - but in that case we fix the bug 
and not the warning - the warning was just the canary to it.)

</soapbox>

	Ingo

[PATCH] compile time warnings

[Ingo Brueckl]

Ingo Molnar <mingo@elte.hu> writes:

 > Would you mind to send a patch for that that we could push to Linus?

Do you mean something like:

fix compiler warning in arch/x86/mm/init_32.c

Signed-off-by: Ingo Brueckl <ib@wupperonline.de>

--- linux-2.6.28/arch/x86/mm/init_32.c.orig	2008-12-25 00:26:37.000000000 +0100
+++ linux-2.6.28/arch/x86/mm/init_32.c	2009-01-02 12:51:17.000000000 +0100
@@ -436,8 +436,13 @@ static void __init set_highmem_pages_ini
 #endif /* !CONFIG_NUMA */

 #else
-# define permanent_kmaps_init(pgd_base)		do { } while (0)
-# define set_highmem_pages_init()	do { } while (0)
+static inline void permanent_kmaps_init(pgd_t *pgd_base)
+{
+	(void) pgd_base;
+}
+static inline void set_highmem_pages_init(void)
+{
+}
 #endif /* CONFIG_HIGHMEM */

 void __init native_pagetable_setup_start(pgd_t *base)

Re: [PATCH] compile time warnings

[Ingo Molnar]

* Ingo Brueckl <ib@wupperonline.de> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
>  > Would you mind to send a patch for that that we could push to Linus?
> 
> Do you mean something like:
> 
> fix compiler warning in arch/x86/mm/init_32.c

yes, with a small nit:
> 
> +static inline void permanent_kmaps_init(pgd_t *pgd_base)
> +{
> +	(void) pgd_base;

there's no need for this line - this is not a macro, so the function 
parameter does not have to be 'used'.

	Ingo

[PATCH] compile time warnings

[Ingo Brueckl]

Ingo Molnar <mingo@elte.hu> writes:

 > yes, with a small nit:
>>
>> +static inline void permanent_kmaps_init(pgd_t *pgd_base)
>> +{
>> +     (void) pgd_base;

 > there's no need for this line - this is not a macro, so the function
 > parameter does not have to be 'used'.

I live and learn.


fix compiler warning in arch/x86/mm/init_32.c

Signed-off-by: Ingo Brueckl <ib@wupperonline.de>

--- linux-2.6.28/arch/x86/mm/init_32.c.orig	2008-12-25 00:26:37.000000000 +0100
+++ linux-2.6.28/arch/x86/mm/init_32.c	2009-01-02 14:21:18.000000000 +0100
@@ -436,8 +436,12 @@ static void __init set_highmem_pages_ini
 #endif /* !CONFIG_NUMA */

 #else
-# define permanent_kmaps_init(pgd_base)		do { } while (0)
-# define set_highmem_pages_init()	do { } while (0)
+static inline void permanent_kmaps_init(pgd_t *pgd_base)
+{
+}
+static inline void set_highmem_pages_init(void)
+{
+}
 #endif /* CONFIG_HIGHMEM */

 void __init native_pagetable_setup_start(pgd_t *base)

Re: [PATCH] compile time warnings

[Ingo Molnar]

* Ingo Brueckl <ib@wupperonline.de> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
>  > yes, with a small nit:
> >>
> >> +static inline void permanent_kmaps_init(pgd_t *pgd_base)
> >> +{
> >> +     (void) pgd_base;
> 
>  > there's no need for this line - this is not a macro, so the function
>  > parameter does not have to be 'used'.
> 
> I live and learn.
> 
> fix compiler warning in arch/x86/mm/init_32.c

applied to tip/x86/cleanups, thanks! Find below the final form of the 
commit.

	Ingo

---------------->
>From a9067d537615d534dcef06c0d819472e43a0d152 Mon Sep 17 00:00:00 2001
From: Ingo Brueckl <ib@wupperonline.de>
Date: Fri, 2 Jan 2009 14:42:00 +0100
Subject: [PATCH] x86: convert permanent_kmaps_init() from macro to inline

Impact: cleanup

This compiler warning:

  arch/x86/mm/init_32.c:515: warning: unused variable 'pgd_base'

triggers because permanent_kmaps_init() is a CPP macro in the
!CONFIG_HIGHMEM case, that does not tell the compiler that the
'pgd_base' parameter is used.

Convert permanent_kmaps_init() (and set_highmem_pages_init()) to
C inline functions - which gives the parameter a proper type and
which gets rid of the compiler warning as well.

Signed-off-by: Ingo Brueckl <ib@wupperonline.de>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/mm/init_32.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 800e1d9..ad98b18 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -434,8 +434,12 @@ static void __init set_highmem_pages_init(void)
 #endif /* !CONFIG_NUMA */
 
 #else
-# define permanent_kmaps_init(pgd_base)		do { } while (0)
-# define set_highmem_pages_init()	do { } while (0)
+static inline void permanent_kmaps_init(pgd_t *pgd_base)
+{
+}
+static inline void set_highmem_pages_init(void)
+{
+}
 #endif /* CONFIG_HIGHMEM */
 
 void __init native_pagetable_setup_start(pgd_t *base)

Re: compile time warnings

[Jesper Juhl]

On Fri, 2 Jan 2009, Ingo Molnar wrote:
> 
> * Jesper Juhl <jj@chaosbits.net> wrote:
> 
> > On Fri, 2 Jan 2009, Tom Spink wrote:
> > 
> > > 2009/1/1 Jesper Juhl <jj@chaosbits.net>:
> > > > On Thu, 1 Jan 2009, Ingo Brueckl wrote:
> > > [snip]
> > > 
> > > Hi,
> > > 
> > > > pgd_base is very much used...
> > > 
> > > It's probably something to do with:
> > > 
> > > # define permanent_kmaps_init(pgd_base)         do { } while (0)
> > > 
> > > Which is within the #else part of #if CONFIG_HIGHMEM.  So, if
> > > CONFIG_HIGHMEM is not set, permanent_kmaps_init gets wiped out, and
> > > therefore that warning will be issued.
> > > 
> > > Perhaps changing that to an empty inline would remove the warning?
> > > 
> > Yeah, I noticed that as well after sending the mail.
> > Another way to silence the warning (which I think is nicer) would be 
> > something like this;
> > 
> > 
> > Silence 'unused variable' warning in arch/x86/mm/init_32.c::pagetable_init
> > 
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> > ---
> > 
> > diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> > index 8655b5b..0affa8e 100644
> > --- a/arch/x86/mm/init_32.c
> > +++ b/arch/x86/mm/init_32.c
> > @@ -511,9 +511,7 @@ static void __init early_ioremap_page_table_range_init(pgd_t *pgd_base)
> >  
> >  static void __init pagetable_init(void)
> >  {
> > -	pgd_t *pgd_base = swapper_pg_dir;
> > -
> > -	permanent_kmaps_init(pgd_base);
> > +	permanent_kmaps_init((pgd_t *)swapper_pg_dir);
> >  }
> 
> no, this only works around the warning by 'silencing' it (it also includes 
> an ugly type cast) - instead of fixing the core problem.
> 
Hmm, true..

> The core problem is that permanent_kmaps_init() is a CPP macro in the 
> !HIGHMEM case - so the right fix would be to convert that to a proper C 
> inline function. (same for set_highmem_pages_init() while at it)
> 
> Would you mind to send a patch for that that we could push to Linus?
>
Sure, I'll do that.
 
> <soapbox>
> 
> The highest quality fixes that are motivated by compiler warnings are the 
> ones that do not actually 'fix a warning', but instead improve/reshape 
> some code so that as a side-effect the warning goes away.
> 
> If you ever see a patch that 'silences a warning', it usually shows that 
> the deeper problem has not been fully understood. (Except of course if the 
> warning shows a genuine bug in the code - but in that case we fix the bug 
> and not the warning - the warning was just the canary to it.)
> 
> </soapbox>
> 
Well said.

> 	Ingo
> 

-- 
Jesper Juhl <jj@chaosbits.net>
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

Re: compile time warnings

[Jesper Juhl]

On Fri, 2 Jan 2009, Jesper Juhl wrote:

> On Fri, 2 Jan 2009, Ingo Molnar wrote:
> > 
...
> > Would you mind to send a patch for that that we could push to Linus?
> >
> Sure, I'll do that.
>  
I see that Mr. Brueckl beat me to it...

-- 
Jesper Juhl <jj@chaosbits.net>
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

Re: compile time warnings

[Ingo Brueckl]

What we have so far:

  arch/x86/kernel/setup.c:742: warning: 'dmi_low_memory_corruption'

is already fixed in the git tree.

  arch/x86/mm/init_32.c:515: warning: unused variable 'pgd_base'

could be fixed with Jespers patch (which I like best).

  WARNING: modpost: Found 1 section mismatch(es).

which is (with CONFIG_DEBUG_SECTION_MISMATCH=y):

  WARNING: vmlinux.o(.cpuinit.data+0x0): Section mismatch in reference from
  the variable initial_code to the function .init.text:i386_start_kernel()
  The variable __cpuinitdata initial_code references a function __init
  i386_start_kernel(). If i386_start_kernel is only used by initial_code then
  annotate i386_start_kernel with a matching annotation.

was already noticed some time ago by linuxtwid...@gmail.com, but is still
unfixed.

Here is what linuxtwid...@gmail.com suggested:

--- linux-2.6.27.1/arch/x86/kernel/head_32.S	2008-10-16 00:49:00.000000000 -0500
+++ linux-2.6.27.1-test/arch/x86/kernel/head_32.S	2008-10-16 01:45:10.000000000 -0500
@@ -600,6 +600,7 @@ ignore_int:

 .section .cpuinit.data,"wa"
 .align 4
+__REFDATA
 ENTRY(initial_code)
 	.long i386_start_kernel

The remaining

  /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds

is very strange and a mystery to me. (There were already reports on that
issue which seem to point to a gcc bug.)

Re: compile time warnings

[Robert Hancock]

Ingo Brueckl wrote:
> Maybe somebody noticed already, with kernel 2.6.28 and gcc 4.3.2 there are a
> few compile time warnings:
>   drivers/acpi/tables/tbfadt.c: In function 'acpi_tb_create_local_fadt':
>   /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds

I noticed this as well. This one looks like the compiler's getting a bit 
confused.

	ACPI_MEMCPY(&acpi_gbl_FADT, table,
		    ACPI_MIN(length, sizeof(struct acpi_table_fadt)));

and somehow that falls into __constant_memcpy because somehow 
__builtin_constant_p() on the length parameter returns true. Huh? It's a 
non-static function where length is passed in, and ACPI_MIN is a simple 
(((a)<(b))?(a):(b)) expression. How can it think that's a compile time 
constant I don't know. Maybe it's not and just generating warnings from 
the code path it's seeing but not using?

The cause of the complaint might be the fact that the memcpy may appear 
to read past the end of the "table" structure which points to struct 
acpi_table_header. Presumably that memory is bigger than the actual 
struct acpi_table_header though. The text of the warning itself is 
bizarre though, as there is no array style access happening, and I don't 
see how the compiler can know what the actual bounds of the "array" are.

> 
>   drivers/usb/core/hcd.c: In function 'usb_hcd_poll_rh_status':
>   /usr/src/linux/arch/x86/include/asm/string_32.h:75: warning: array subscript is above array bounds

This one makes a little more sense. This is presumably:

			memcpy(urb->transfer_buffer, buffer, length);

where transfer_buffer is a void*, buffer is char[4] and length comes 
from hcd->driver->hub_status_data(hcd, buffer) which I assume fills in 
the buffer. It's true the memcpy could read past the end of the buffer 
array, but only if the hub_status_data could fill in and return a length 
greater than 4, which it hopefully can't..