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'
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 <[email protected]>
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
2009/1/1 Jesper Juhl <[email protected]>:
> 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."
On Fri, 2009-01-02 at 01:35 +0000, Tom Spink wrote:
> 2009/1/1 Jesper Juhl <[email protected]>:
> > 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
On Fri, 2 Jan 2009, Tom Spink wrote:
> 2009/1/1 Jesper Juhl <[email protected]>:
> > 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 <[email protected]>
---
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 <[email protected]>
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
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 [email protected], but is still
unfixed.
Here is what [email protected] 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.)
* Jesper Juhl <[email protected]> wrote:
> On Fri, 2 Jan 2009, Tom Spink wrote:
>
> > 2009/1/1 Jesper Juhl <[email protected]>:
> > > 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 <[email protected]>
> ---
>
> 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
Ingo Molnar <[email protected]> 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 <[email protected]>
--- 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)
* Ingo Brueckl <[email protected]> wrote:
> Ingo Molnar <[email protected]> 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
Ingo Molnar <[email protected]> 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 <[email protected]>
--- 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)
* Ingo Brueckl <[email protected]> wrote:
> Ingo Molnar <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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)
On Fri, 2 Jan 2009, Ingo Molnar wrote:
>
> * Jesper Juhl <[email protected]> wrote:
>
> > On Fri, 2 Jan 2009, Tom Spink wrote:
> >
> > > 2009/1/1 Jesper Juhl <[email protected]>:
> > > > 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
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
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 <[email protected]>
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
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..