[RFC] Error out on implicit declarations

[Carl-Daniel Hailfinger]

Bao, Zheng found a bug which killed SATA booting on my board.

This happened because we do not error out on implicit function
declarations. The linker has no way of checking whether the implicitly
assumed function signature is identical to the real signature, so
mismatches can occur and these mismatches are practically impossible to
debug because the code looks completely correct.

Adding -Werror-implicit-function-declaration to our CFLAGS would solve
this problem nicely, but a lot of files in the tree need to be fixed.

Example set of errors for the AMD DBM690T target:
src/cpu/amd/model_fxx/../../../northbridge/amd/amdk8/amdk8_f.h:586:
warning: implicit declaration of function ‘hard_reset’
src/mainboard/amd/dbm690t/fadt.c:71: warning: implicit declaration of
function ‘pm_iowrite’
src/mainboard/amd/dbm690t/mainboard.c:145: warning: implicit declaration
of function ‘pm2_ioread’
src/mainboard/amd/dbm690t/mainboard.c:147: warning: implicit declaration
of function ‘pm2_iowrite’
src/mainboard/amd/dbm690t/mainboard.c:164: warning: implicit declaration
of function ‘pm_ioread’
src/mainboard/amd/dbm690t/mainboard.c:166: warning: implicit declaration
of function ‘pm_iowrite’
src/mainboard/amd/dbm690t/mainboard.c:255: warning: implicit declaration
of function ‘lb_add_memory_range’
src/northbridge/amd/amdk8/amdk8_f.h:586: warning: implicit declaration
of function ‘hard_reset’
src/southbridge/amd/sb600/../../../northbridge/amd/amdk8/reset_test.c:16:
warning: implicit declaration of function ‘pci_read_config32’
src/southbridge/amd/sb600/../../../northbridge/amd/amdk8/reset_test.c:44:
warning: implicit declaration of function ‘pci_write_config32’
src/superio/ite/it8712f/superio.c:80: warning: implicit declaration of
function ‘set_kbc_ps2_mode’

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv2-asus_m2a-vm/src/config/Config.lb
===================================================================
--- LinuxBIOSv2-asus_m2a-vm/src/config/Config.lb	(Revision 3816)
+++ LinuxBIOSv2-asus_m2a-vm/src/config/Config.lb	(Arbeitskopie)
@@ -8,7 +8,7 @@
 makedefine GCC_INC_DIR := $(shell LC_ALL=C $(CC) -print-search-dirs | sed -ne "s/install: (.*)/\1include/gp")

makedefine CPPFLAGS := -I$(TOP)/src/include -I$(TOP)/src/arch/$(ARCH)/include -I$(GCC_INC_DIR) $(CPUFLAGS)
-makedefine CFLAGS := $(CPU_OPT) $(DISTRO_CFLAGS) $(CPPFLAGS) -Os -nostdinc -nostdlib -fno-builtin  -Wall
+makedefine CFLAGS := $(CPU_OPT) $(DISTRO_CFLAGS) $(CPPFLAGS) -Os -nostdinc -nostdlib -fno-builtin  -Wall -Werror-implicit-function-declaration

makedefine HOSTCFLAGS:= -Os -Wall

[Myles Watson]

On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Bao, Zheng found a bug which killed SATA booting on my board.
>
> This happened because we do not error out on implicit function
> declarations. The linker has no way of checking whether the implicitly
> assumed function signature is identical to the real signature, so
> mismatches can occur and these mismatches are practically impossible to
> debug because the code looks completely correct.
>
> Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> this problem nicely, but a lot of files in the tree need to be fixed.

I think this is a great idea.  Isn't the correct order to fix all the
warnings, then make it an error?

Thanks,
Myles

[Jordan Crouse]

Myles Watson wrote:
> On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> [e-mail]> wrote:
> > Bao, Zheng found a bug which killed SATA booting on my board.
> >
> > This happened because we do not error out on implicit function
> > declarations. The linker has no way of checking whether the implicitly
> > assumed function signature is identical to the real signature, so
> > mismatches can occur and these mismatches are practically impossible to
> > debug because the code looks completely correct.
> >
> > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > this problem nicely, but a lot of files in the tree need to be fixed.
>
> I think this is a great idea.  Isn't the correct order to fix all the
> warnings, then make it an error?

Yeah - the unfortunate thing about changes like this is that you end up 
being responsible for fixing the errors.. :)

Carl-Daniel - if you post a list of offending files, we'll all help 
clear them up.  Dumping the log through grep "implicit declaration of 
function" should suffice.

Jordan

[ron minnich]

I like this idea a lot, I have no idea why we did not have that
enabled before ... I'm glad I'm on vacation and you all can fix it :-)

thanks

ron

[Corey Osgood]

On Thu, Dec 18, 2008 at 10:58 AM, ron minnich  wrote:
> I like this idea a lot, I have no idea why we did not have that
> enabled before ... I'm glad I'm on vacation and you all can fix it :-)

I'm all for it. CN700 should be all ready for the change.

-Corey

[Corey Osgood]

On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood wrote:
> On Thu, Dec 18, 2008 at 10:58 AM, ron minnich  wrote:
> > I like this idea a lot, I have no idea why we did not have that
> > enabled before ... I'm glad I'm on vacation and you all can fix it :-)
>
> I'm all for it. CN700 should be all ready for the change.

I retract that, I was thinking of v3. Attached patch fixes v2.

-Corey

[Myles Watson]

On Thu, Dec 18, 2008 at 12:24 PM, Corey Osgood wrote:
> On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood wrote:
> > On Thu, Dec 18, 2008 at 10:58 AM, ron minnich  wrote:
> > > I like this idea a lot, I have no idea why we did not have that
> > > enabled before ... I'm glad I'm on vacation and you all can fix it :-)
> >
> > I'm all for it. CN700 should be all ready for the change.
>
> I retract that, I was thinking of v3. Attached patch fixes v2.

Index: src/northbridge/via/cn700/vgachip.h
===================================================================
--- src/northbridge/via/cn700/vgachip.h (revision 3818)
+++ src/northbridge/via/cn700/vgachip.h (working copy)
@@ -1,6 +1,6 @@
 /*
 * This file is part of the coreboot project.
- *
+ *;

I'm curious about this extra semicolon.  Besides that:

Acked-by: Myles Watson

Thanks,
Myles

[Corey Osgood]

On Thu, Dec 18, 2008 at 2:28 PM, Myles Watson  wrote:
> On Thu, Dec 18, 2008 at 12:24 PM, Corey Osgood wrote:
> > On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood wrote:
> > > On Thu, Dec 18, 2008 at 10:58 AM, ron minnich wrote:
> > > > I like this idea a lot, I have no idea why we did not have that
> > > > enabled before ... I'm glad I'm on vacation and you all can fix it :-)
> > >
> > > I'm all for it. CN700 should be all ready for the change.
> >
> > I retract that, I was thinking of v3. Attached patch fixes v2.
> Index: src/northbridge/via/cn700/vgachip.h
>
> --- src/northbridge/via/cn700/vgachip.h (revision 3818)
> +++ src/northbridge/via/cn700/vgachip.h (working copy)
> @@ -1,6 +1,6 @@
>  /*
> This file is part of the coreboot project.
> *;
>
> I'm curious about this extra semicolon.  Besides that:
>
> Acked-by: Myles Watson

Ah, I was looking for that! I have a bad habit of hitting my touchpad while
typing and moving the cursor. Thanks, r3819

-Corey

[Corey Osgood]

On Thu, Dec 18, 2008 at 2:38 PM, Corey Osgood wrote:
> On Thu, Dec 18, 2008 at 2:28 PM, Myles Watson  wrote:
> > On Thu, Dec 18, 2008 at 12:24 PM, Corey Osgood wrote:
> > > On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood wrote:
> > > > On Thu, Dec 18, 2008 at 10:58 AM, ron minnich wrote:
> > > > > I like this idea a lot, I have no idea why we did not have that
> > > > > enabled before ... I'm glad I'm on vacation and you all can fix it :-)
> > > >
> > > > I'm all for it. CN700 should be all ready for the change.
> > >
> > > I retract that, I was thinking of v3. Attached patch fixes v2.
> > Index: src/northbridge/via/cn700/vgachip.h
> >
> > --- src/northbridge/via/cn700/vgachip.h (revision 3818)
> > +++ src/northbridge/via/cn700/vgachip.h (working copy)
> > @@ -1,6 +1,6 @@
> >  /*
> > This file is part of the coreboot project.
> > *;
> >
> > I'm curious about this extra semicolon.  Besides that:
> >
> > Acked-by: Myles Watson
>
> Ah, I was looking for that! I have a bad habit of hitting my touchpad while
> typing and moving the cursor. Thanks, r3819

I just got another implicit declaration in i82810, and i82801xx is clean.
Someone ought to check i440bx, i830, and the other via chipsets, vt8623 will
probably have the same ones cn700 did, but I've got to get back to work.

-Corey

[Corey Osgood]

On Thu, Dec 18, 2008 at 2:56 PM, Corey Osgood wrote:
> On Thu, Dec 18, 2008 at 2:38 PM, Corey Osgood wrote:
> > On Thu, Dec 18, 2008 at 2:28 PM, Myles Watson  wrote:
> > > On Thu, Dec 18, 2008 at 12:24 PM, Corey Osgood wrote:
> > > > On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood wrote:
> > > > > On Thu, Dec 18, 2008 at 10:58 AM, ron minnich wrote:
> > > > > > I like this idea a lot, I have no idea why we did not have that
> > > > > > enabled before ... I'm glad I'm on vacation and you all can fix it :-)
> > > > >
> > > > > I'm all for it. CN700 should be all ready for the change.
> > > >
> > > > I retract that, I was thinking of v3. Attached patch fixes v2.
> > > Index: src/northbridge/via/cn700/vgachip.h
> > >
> > > --- src/northbridge/via/cn700/vgachip.h (revision 3818)
> > > +++ src/northbridge/via/cn700/vgachip.h (working copy)
> > > @@ -1,6 +1,6 @@
> > >  /*
> > > This file is part of the coreboot project.
> > > *;
> > >
> > > I'm curious about this extra semicolon.  Besides that:
> > >
> > > Acked-by: Myles Watson
> >
> > Ah, I was looking for that! I have a bad habit of hitting my touchpad
> > while typing and moving the cursor. Thanks, r3819
>
> I just got another implicit declaration in i82810, and i82801xx is clean.
> Someone ought to check i440bx, i830, and the other via chipsets, vt8623 will
> probably have the same ones cn700 did, but I've got to get back to work.

Here's another patch to clear up a bunch more, not all of them by far. I'm
for the most part avoiding k8 and sticking with the simpler ones.

-Corey

[Myles Watson]

On Thu, Dec 18, 2008 at 3:04 PM, Corey Osgood wrote:
> On Thu, Dec 18, 2008 at 2:56 PM, Corey Osgood wrote:
> > On Thu, Dec 18, 2008 at 2:38 PM, Corey Osgood wrote:
> > > On Thu, Dec 18, 2008 at 2:28 PM, Myles Watson  wrote:
> > > > On Thu, Dec 18, 2008 at 12:24 PM, Corey Osgood wrote:
> > > > > On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood <[e-mail]
> > > > > > wrote:
> > > > > > On Thu, Dec 18, 2008 at 10:58 AM, ron minnich wrote:
> > > > > >
> > > > > > > I like this idea a lot, I have no idea why we did not have that
> > > > > > > enabled before ... I'm glad I'm on vacation and you all can fix it
> > > > > > > :-)
> > > > > >
> > > > > > I'm all for it. CN700 should be all ready for the change.
> > > > >
> > > > > I retract that, I was thinking of v3. Attached patch fixes v2.
> > > > Index: src/northbridge/via/cn700/vgachip.h
> > > >
> > > > --- src/northbridge/via/cn700/vgachip.h (revision 3818)
> > > > +++ src/northbridge/via/cn700/vgachip.h (working copy)
> > > > @@ -1,6 +1,6 @@
> > > >  /*
> > > > This file is part of the coreboot project.
> > > > *;
> > > >
> > > > I'm curious about this extra semicolon.  Besides that:
> > > >
> > > > Acked-by: Myles Watson
> > >
> > > Ah, I was looking for that! I have a bad habit of hitting my touchpad
> > > while typing and moving the cursor. Thanks, r3819
> >
> > I just got another implicit declaration in i82810, and i82801xx is clean.
> > Someone ought to check i440bx, i830, and the other via chipsets, vt8623 will
> > probably have the same ones cn700 did, but I've got to get back to work.
>
> Here's another patch to clear up a bunch more, not all of them by far. I'm
> for the most part avoiding k8 and sticking with the simpler ones.

Acked-by: Myles Watson

Thanks,
Myles

[Corey Osgood]

On Thu, Dec 18, 2008 at 10:02 PM, Myles Watson  wrote:
> On Thu, Dec 18, 2008 at 3:04 PM, Corey Osgood wrote:
> > On Thu, Dec 18, 2008 at 2:56 PM, Corey Osgood wrote:
> > > On Thu, Dec 18, 2008 at 2:38 PM, Corey Osgood wrote:
> > > > On Thu, Dec 18, 2008 at 2:28 PM, Myles Watson wrote:
> > > > > On Thu, Dec 18, 2008 at 12:24 PM, Corey Osgood <[e-mail]
> > > > > > wrote:
> > > > > > On Thu, Dec 18, 2008 at 11:37 AM, Corey Osgood <
> > > > > > [e-mail]> wrote:
> > > > > >
> > > > > > > On Thu, Dec 18, 2008 at 10:58 AM, ron minnich wrote:
> > > > > > >
> > > > > > >> I like this idea a lot, I have no idea why we did not have that
> > > > > > >> enabled before ... I'm glad I'm on vacation and you all can fix it
> > > > > > >> :-)
> > > > > > >
> > > > > > >
> > > > > > > I'm all for it. CN700 should be all ready for the change.
> > > > > > >
> > > > > >
> > > > > > I retract that, I was thinking of v3. Attached patch fixes v2.
> > > > > Index: src/northbridge/via/cn700/vgachip.h
> > > > >
> > > > > --- src/northbridge/via/cn700/vgachip.h (revision 3818)
> > > > > +++ src/northbridge/via/cn700/vgachip.h (working copy)
> > > > > @@ -1,6 +1,6 @@
> > > > >  /*
> > > > > This file is part of the coreboot project.
> > > > > *;
> > > > >
> > > > > I'm curious about this extra semicolon.  Besides that:
> > > > >
> > > > > Acked-by: Myles Watson
> > > >
> > > > Ah, I was looking for that! I have a bad habit of hitting my touchpad
> > > > while typing and moving the cursor. Thanks, r3819
> > >
> > > I just got another implicit declaration in i82810, and i82801xx is clean.
> > > Someone ought to check i440bx, i830, and the other via chipsets, vt8623 will
> > > probably have the same ones cn700 did, but I've got to get back to work.
> >
> > Here's another patch to clear up a bunch more, not all of them by far. I'm
> > for the most part avoiding k8 and sticking with the simpler ones.
>
> Acked-by: Myles Watson

r3822, thanks

-Corey

[Li, Maggie]

Hi,

Here are the patches for fixing implicit declarations in amdk8, dbm690t and pistachio.

Please help me check it. Thanks.

Maggie li

[Bao, Zheng]

Signed-off-by: Maggie Li [e-mail]

Reviewed-by: Zheng bao

________________________________

From: [e-mail]
[mailto:[e-mail]] On Behalf Of Li, Maggie
Sent: Friday, December 19, 2008 4:56 PM
To: Coreboot
Subject: Re: [coreboot] [RFC] Error out on implicit declarations

Hi,

Here are the patches for fixing implicit declarations in amdk8, dbm690t
and pistachio.

Please help me check it. Thanks.

Maggie li

[Corey Osgood]

On Fri, Dec 19, 2008 at 4:12 AM, Bao, Zheng  wrote:
> Signed-off-by: Maggie Li [e-mail]
>
> Reviewed-by: Zheng bao
>
> *From:* [e-mail] [mailto:
> [e-mail]] *On Behalf Of *Li, Maggie
> *Sent:* Friday, December 19, 2008 4:56 PM
> *To:* Coreboot
> *Subject:* Re: [coreboot] [RFC] Error out on implicit declarations
>
> Hi,
>
> Here are the patches for fixing implicit declarations in amdk8, dbm690t and
> pistachio.
>
> Please help me check it. Thanks.

Thanks for the patches, but I think they've broken other boards, and I'd
rather not check in a patch that breaks what currently builds. The main ones
I'm at are the agami aruma:

In file included from
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/../../../northbridge/amd/amdk8/reset_test.c:3,
                 from
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:54:
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:74:1:
warning: "PCI_DEV" redefined
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:4:1:
warning: this is the location of the previous definition
In file included from
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/../../../northbridge/amd/amdk8/reset_test.c:3,
                 from
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:54:
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:85:
error: redefinition of typedef 'device_t'
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:12:
error: previous declaration of 'device_t' was here
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:174:
error: redefinition of 'pci_read_config32'
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:31:
error: previous definition of 'pci_read_config32' was here
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:204:
error: redefinition of 'pci_write_config8'
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:15:
error: previous definition of 'pci_write_config8' was here
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:266:
error: redefinition of 'pci_write_config32'
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:23:
error: previous definition of 'pci_write_config32' was here
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:300:
error: redefinition of 'pci_locate_device_on_bus'
/home/corey/coreboot/coreboot-v2/src/southbridge/amd/amd8111/amd8111_reset.c:40:
error: previous definition of 'pci_locate_device_on_bus' was here
make[1]: *** [amd8111_reset.o] Error 1

and asus a8n_e:

/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:174:
error: redefinition of 'pci_read_config32'
/home/corey/coreboot/coreboot-v2/src/southbridge/nvidia/ck804/ck804_reset.c:24:
error: previous definition of 'pci_read_config32' was here
/home/corey/coreboot/coreboot-v2/src/arch/i386/include/arch/romcc_io.h:266:
error: redefinition of 'pci_write_config32'
/home/corey/coreboot/coreboot-v2/src/southbridge/nvidia/ck804/ck804_reset.c:16:
error: previous definition of 'pci_write_config32' was here

The fix is probably as simple as multiple inclusion guards in the header
files. I'd do it, but it's 4:30am here, I'm headed to bed. I'm also
attaching the log of an abuild run with your patches and Carl-Daniel's
applied, so everyone can see where we stand. Perhaps we should create a
temporary wiki page with the list of broken targets in it, and check them
off as they're fixed? Only downside being not everyone has wiki access...

-Corey

[Myles Watson]

These two patches fix a few more implicit declarations, and fix the build
for the dell board that got broken.

Signed-off-by: Myles Watson

Thanks,
Myles

[Joseph Smith]

Arrh,
Looks like both of the 82830 boards are broken. What do I need to do to fix
them?

Processing mainboard/rca/rm4100 (i386: ok)
Using existing test target
/home/corey/coreboot/coreboot-v2/targets/rca/rm4100/Config-abuild.lb ok
  Creating builddir...ok
  Compiling image on 2 cpus in parallel .. FAILED after 10s! Log excerpt:
make[1]: *** [northbridge.o] Error 1
make[1]: *** Waiting for unfinished jobs....
mv romcc.tmpfile ../romcc
make[1]: Leaving directory
`/home/corey/coreboot/coreboot-v2/util/abuild/coreboot-builds/rca_rm4100/fallback'
make: *** [fallback/coreboot.rom] Error 1

Processing mainboard/thomson/ip1000 (i386: ok)
Using existing test target
/home/corey/coreboot/coreboot-v2/targets/thomson/ip1000/Config-abuild.lb ok
  Creating builddir...ok
  Compiling image on 2 cpus in parallel .. FAILED after 10s! Log excerpt:
make[1]: *** [northbridge.o] Error 1
make[1]: *** Waiting for unfinished jobs....
mv romcc.tmpfile ../romcc
make[1]: Leaving directory
`/home/corey/coreboot/coreboot-v2/util/abuild/coreboot-builds/thomson_ip1000/fallback'
make: *** [fallback/coreboot.rom] Error 1

[Myles Watson]

On Fri, Dec 19, 2008 at 11:03 AM, Joseph Smith  wrote:
> Arrh,
> Looks like both of the 82830 boards are broken. What do I need to do to fix
> them?

Do they still break after my last two patches?  I thought they were fixed.

Thanks,
Myles

[Joseph Smith]

On Fri, 19 Dec 2008 11:16:19 -0700, "Myles Watson" 
wrote:
> On Fri, Dec 19, 2008 at 11:03 AM, Joseph Smith

wrote:
> > Arrh,
> > Looks like both of the 82830 boards are broken. What do I need to do to
>
> fix
> > them?
>
> Do they still break after my last two patches?  I thought they were

fixed.
>

I was going by Corey's last abuild log. I appologize if you have fixed them
since then.

[Corey Osgood]

On Fri, Dec 19, 2008 at 1:19 PM, Joseph Smith  wrote:
> On Fri, 19 Dec 2008 11:16:19 -0700, "Myles Watson" 
> wrote:
> > On Fri, Dec 19, 2008 at 11:03 AM, Joseph Smith
>
> wrote:
> > > Arrh,
> > > Looks like both of the 82830 boards are broken. What do I need to do to
> >
> > fix
> > > them?
> >
> > Do they still break after my last two patches?  I thought they were
>
> fixed.
> >
>
> I was going by Corey's last abuild log. I appologize if you have fixed them
> since then.

rca/rm4100 is  still broken. All you should need to do is check that all
the functions are properly declared, either by including their declarations
in a header or an extern(al) declaration. Making the change in Carl-Daniel's
patch will tell you very quickly if you got them all or not ;)

Myles, those patches when applied with Maggi's break the amd pistachio and
dbm690t. New abuild log with all 6? patches applied. I accidentally ran it a
second time and lost the original log, so don't mind the "previously ok"
comments, those boards were actually rebuilt and succeeded once.

-Corey

[Myles Watson]

On Fri, Dec 19, 2008 at 12:16 PM, Corey Osgood wrote:
> On Fri, Dec 19, 2008 at 1:19 PM, Joseph Smith  wrote:
> > On Fri, 19 Dec 2008 11:16:19 -0700, "Myles Watson" 
> > wrote:
> > > On Fri, Dec 19, 2008 at 11:03 AM, Joseph Smith
> >
> > wrote:
> > > > Arrh,
> > > > Looks like both of the 82830 boards are broken. What do I need to do to
> > >
> > > fix
> > > > them?
> > >
> > > Do they still break after my last two patches?  I thought they were
> >
> > fixed.
> > >
> >
> > I was going by Corey's last abuild log. I appologize if you have fixed
> > them
> > since then.
>
> I don't know what was going on.  abuild ran fine for me.  I ran it again

and sure enough, it breaks.  Sorry about that.
> rca/rm4100 is  still broken. All you should need to do is check that all
> the functions are properly declared, either by including their declarations
> in a header or an extern(al) declaration. Making the change in Carl-Daniel's
> patch will tell you very quickly if you got them all or not ;)
>
> Myles, those patches when applied with Maggi's break the amd pistachio and
> dbm690t. New abuild log with all 6? patches applied. I accidentally ran it a
> second time and lost the original log, so don't mind the "previously ok"
> comments, those boards were actually rebuilt and succeeded once.

I didn't try with Maggi's patches because you said they broke other boards.

Thanks,
Myles

[Myles Watson]

On Thu, Dec 18, 2008 at 7:52 AM, Jordan Crouse wrote:
> Myles Watson wrote:
> > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > [e-mail]> wrote:
> >
> > Bao, Zheng found a bug which killed SATA booting on my board.
> > > This happened because we do not error out on implicit function
> > > declarations. The linker has no way of checking whether the implicitly
> > > assumed function signature is identical to the real signature, so
> > > mismatches can occur and these mismatches are practically impossible to
> > > debug because the code looks completely correct.
> > >
> > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > this problem nicely, but a lot of files in the tree need to be fixed.
> >
> > I think this is a great idea.  Isn't the correct order to fix all the
> > warnings, then make it an error?
>
> Yeah - the unfortunate thing about changes like this is that you end up
> being responsible for fixing the errors.. :)

Here's my first patch.  It clears up all of them except get_nodes for
serengeti.

coreboot/svn/src/cpu/amd/dualcore/dualcore.c:63: warning: implicit
declaration of function 'get_nodes'

The rest were easy.  This one I'm not sure what was supposed to be here.
> Carl-Daniel - if you post a list of offending files, we'll all help clear
> them up.  Dumping the log through grep "implicit declaration of function"
> should suffice.

If you want to take the get_nodes reference, that would be great.  If this
is the way its supposed to be cleaned up, I'll keep going a little more.  I
think we should divide it up based on processor type so we don't duplicate
work.

Signed-off-by: Myles Watson

Thanks,
Myles

[Marc Jones]

On Thu, Dec 18, 2008 at 10:22 AM, Myles Watson  wrote:
> On Thu, Dec 18, 2008 at 7:52 AM, Jordan Crouse 
> wrote:
> > Myles Watson wrote:
> > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > [e-mail]> wrote:
> > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > >
> > > > This happened because we do not error out on implicit function
> > > > declarations. The linker has no way of checking whether the implicitly
> > > > assumed function signature is identical to the real signature, so
> > > > mismatches can occur and these mismatches are practically impossible to
> > > > debug because the code looks completely correct.
> > > >
> > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > this problem nicely, but a lot of files in the tree need to be fixed.
> > >
> > > I think this is a great idea.  Isn't the correct order to fix all the
> > > warnings, then make it an error?
> >
> > Yeah - the unfortunate thing about changes like this is that you end up
> > being responsible for fixing the errors.. :)
>
> Here's my first patch.  It clears up all of them except get_nodes for
> serengeti.
>
> coreboot/svn/src/cpu/amd/dualcore/dualcore.c:63: warning: implicit
> declaration of function 'get_nodes'
>
> The rest were easy.  This one I'm not sure what was supposed to be here.
> > Carl-Daniel - if you post a list of offending files, we'll all help clear
> > them up.  Dumping the log through grep "implicit declaration of function"
> > should suffice.
>
> If you want to take the get_nodes reference, that would be great.  If this
> is the way its supposed to be cleaned up, I'll keep going a little more.  I
> think we should divide it up based on processor type so we don't duplicate
> work.
>
> Signed-off-by: Myles Watson

Acked-by: Marc Jones

This is a much needed cleanup. Thanks Carl-Daniel and Myles for starting on it.
I think that the get_node is a little tricky because  it is used in
CAR and in main coreboot code?

Marc

[Myles Watson]

On Thu, Dec 18, 2008 at 11:08 AM, Marc Jones  wrote:
> On Thu, Dec 18, 2008 at 10:22 AM, Myles Watson  wrote:
> > On Thu, Dec 18, 2008 at 7:52 AM, Jordan Crouse <[e-mail]
> >
> > wrote:
> > > Myles Watson wrote:
> > > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > > [e-mail]> wrote:
> > > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > > >
> > > > > This happened because we do not error out on implicit function
> > > > > declarations. The linker has no way of checking whether the implicitly
> > > > > assumed function signature is identical to the real signature, so
> > > > > mismatches can occur and these mismatches are practically impossible
>
> to
> > > > > debug because the code looks completely correct.
> > > > >
> > > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > > this problem nicely, but a lot of files in the tree need to be fixed.
> > > >
> > > > I think this is a great idea.  Isn't the correct order to fix all the
> > > > warnings, then make it an error?
> > >
> > > Yeah - the unfortunate thing about changes like this is that you end up
> > > being responsible for fixing the errors.. :)
> >
> > Here's my first patch.  It clears up all of them except get_nodes for
> > serengeti.
> >
> > coreboot/svn/src/cpu/amd/dualcore/dualcore.c:63: warning: implicit
> > declaration of function 'get_nodes'
> >
> > The rest were easy.  This one I'm not sure what was supposed to be here.
> > > Carl-Daniel - if you post a list of offending files, we'll all help
>
> clear
> > > them up.  Dumping the log through grep "implicit declaration of
>
> function"
> > > should suffice.
> >
> > If you want to take the get_nodes reference, that would be great.  If
>
> this
> > is the way its supposed to be cleaned up, I'll keep going a little more.
>
> I
> > think we should divide it up based on processor type so we don't
>
> duplicate
> > work.
> >
> > Signed-off-by: Myles Watson
>
> Acked-by: Marc Jones

Rev 3818.  Thanks.
> This is a much needed cleanup. Thanks Carl-Daniel and Myles for starting on
> it.
> I think that the get_node is a little tricky because  it is used in
> CAR and in main coreboot code?

It's also inlined some places but not others.

I'm attaching a patch which I thought was trivial, but breaks things all
over the place.  I'm not sure how this will need to be done.  It looks like
a problem with romcc vs. CAR.

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 18.12.2008 19:33, Myles Watson wrote:
> On Thu, Dec 18, 2008 at 11:08 AM, Marc Jones  wrote:
> > On Thu, Dec 18, 2008 at 10:22 AM, Myles Watson  wrote:
> > > On Thu, Dec 18, 2008 at 7:52 AM, Jordan Crouse <[e-mail]
> > >
> > > wrote:
> > > > Myles Watson wrote:
> > > > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > > > [e-mail]> wrote:
> > > > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > > > >
> > > > > > This happened because we do not error out on implicit function
> > > > > > declarations. The linker has no way of checking whether the implicitly
> > > > > > assumed function signature is identical to the real signature, so
> > > > > > mismatches can occur and these mismatches are practically impossible
> > > > > > to debug because the code looks completely correct.
> > > > > >
> > > > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > > > this problem nicely, but a lot of files in the tree need to be fixed.
> > > > >
> > > > > I think this is a great idea.  Isn't the correct order to fix all the
> > > > > warnings, then make it an error?
> > > >
> > > > Yeah - the unfortunate thing about changes like this is that you end up
> > > > being responsible for fixing the errors.. :)
> > > > Carl-Daniel - if you post a list of offending files, we'll all help
> > > > clear them up.  Dumping the log through grep "implicit 
> > > > declaration of function" should suffice.
>
> I'm attaching a patch which I thought was trivial, but breaks things all
> over the place.  I'm not sure how this will need to be done.  It looks like
> a problem with romcc vs. CAR.

Try this patch. It passes abuild.

Regards,
Carl-Daniel

Fix implicit declarations of pci_read_config8 and pci_write_config8 in
the following files:
src/mainboard/intel/jarrell/reset.c
src/mainboard/supermicro/x6dai_g/reset.c
src/mainboard/supermicro/x6dhe_g2/reset.c
src/mainboard/supermicro/x6dhe_g/reset.c
src/mainboard/supermicro/x6dhr_ig2/reset.c
src/mainboard/supermicro/x6dhr_ig/reset.c

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dai_g/reset.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dai_g/reset.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dai_g/reset.c	(Arbeitskopie)
@@ -3,6 +3,8 @@
 #include <device/pci_ids.h>
 #ifndef __ROMCC__
 #include <device/device.h>
+#include <device/pci.h>
+#include <device/pci_ops.h>
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
    ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 #define PCI_DEV_INVALID 0
Index: LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhe_g/reset.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhe_g/reset.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhe_g/reset.c	(Arbeitskopie)
@@ -3,6 +3,8 @@
 #include <device/pci_ids.h>
 #ifndef __ROMCC__
 #include <device/device.h>
+#include <device/pci.h>
+#include <device/pci_ops.h>
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
    ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 #define PCI_DEV_INVALID 0
Index: LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhe_g2/reset.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhe_g2/reset.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhe_g2/reset.c	(Arbeitskopie)
@@ -3,6 +3,8 @@
 #include <device/pci_ids.h>
 #ifndef __ROMCC__
 #include <device/device.h>
+#include <device/pci.h>
+#include <device/pci_ops.h>
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
    ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 #define PCI_DEV_INVALID 0
Index: LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhr_ig/reset.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhr_ig/reset.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhr_ig/reset.c	(Arbeitskopie)
@@ -3,6 +3,8 @@
 #include <device/pci_ids.h>
 #ifndef __ROMCC__
 #include <device/device.h>
+#include <device/pci.h>
+#include <device/pci_ops.h>
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
    ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 #define PCI_DEV_INVALID 0
Index: LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhr_ig2/reset.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhr_ig2/reset.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/x6dhr_ig2/reset.c	(Arbeitskopie)
@@ -3,6 +3,8 @@
 #include <device/pci_ids.h>
 #ifndef __ROMCC__
 #include <device/device.h>
+#include <device/pci.h>
+#include <device/pci_ops.h>
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
    ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 #define PCI_DEV_INVALID 0
Index: LinuxBIOSv2-implicit_declarations/src/mainboard/intel/jarrell/reset.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/mainboard/intel/jarrell/reset.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/intel/jarrell/reset.c	(Arbeitskopie)
@@ -3,6 +3,8 @@
 #include <device/pci_ids.h>
 #ifndef __ROMCC__
 #include <device/device.h>
+#include <device/pci.h>
+#include <device/pci_ops.h>
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
    ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 #define PCI_DEV_INVALID 0

[Myles Watson]

> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> Sent: Monday, December 22, 2008 6:48 AM
> To: Myles Watson
> Cc: Marc Jones; Coreboot; [e-mail]
> Subject: Re: [coreboot] [RFC] Error out on implicit declarations
>
> On 18.12.2008 19:33, Myles Watson wrote:
> > On Thu, Dec 18, 2008 at 11:08 AM, Marc Jones  wrote:
> > > On Thu, Dec 18, 2008 at 10:22 AM, Myles Watson
>
> wrote:
> > > > On Thu, Dec 18, 2008 at 7:52 AM, Jordan Crouse
>
> <[e-mail]
> > > > wrote:
> > > > > Myles Watson wrote:
> > > > > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > > > > [e-mail]> wrote:
> > > > > >
> > > > > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > > > > >
> > > > > > > This happened because we do not error out on implicit function
> > > > > > > declarations. The linker has no way of checking whether the
>
> implicitly
> > > > > > > assumed function signature is identical to the real signature, so
> > > > > > > mismatches can occur and these mismatches are practically
>
> impossible
> > > > > > > to debug because the code looks completely correct.
> > > > > > >
> > > > > > > Adding -Werror-implicit-function-declaration to our CFLAGS would
>
> solve
> > > > > > > this problem nicely, but a lot of files in the tree need to be
>
> fixed.
> > > > > > >
> > > > > > >
> > > > > > I think this is a great idea.  Isn't the correct order to fix all
>
> the
> > > > > > warnings, then make it an error?
> > > > >
> > > > > Yeah - the unfortunate thing about changes like this is that you end
>
> up
> > > > > being responsible for fixing the errors.. :)
> > > > > Carl-Daniel - if you post a list of offending files, we'll all help
> > > > > clear them up.  Dumping the log through grep "implicit
> > > > > declaration of function" should suffice.
> >
> > I'm attaching a patch which I thought was trivial, but breaks things all
> > over the place.  I'm not sure how this will need to be done.  It looks
>
> like
> > a problem with romcc vs. CAR.
>
> Try this patch. It passes abuild.
>
> Regards,
> Carl-Daniel
>
> Fix implicit declarations of pci_read_config8 and pci_write_config8 in
> the following files:
> src/mainboard/intel/jarrell/reset.c
> src/mainboard/supermicro/x6dai_g/reset.c
> src/mainboard/supermicro/x6dhe_g2/reset.c
> src/mainboard/supermicro/x6dhe_g/reset.c
> src/mainboard/supermicro/x6dhr_ig2/reset.c
> src/mainboard/supermicro/x6dhr_ig/reset.c
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Myles Watson

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 22.12.2008 17:14, Myles Watson wrote:
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> > Sent: Monday, December 22, 2008 6:48 AM
> > To: Myles Watson
> > Cc: Marc Jones; Coreboot; [e-mail]
> > Subject: Re: [coreboot] [RFC] Error out on implicit declarations
> >
> > On 18.12.2008 19:33, Myles Watson wrote:
> > > On Thu, Dec 18, 2008 at 11:08 AM, Marc Jones  wrote:
> > > > On Thu, Dec 18, 2008 at 10:22 AM, Myles Watson
> >
> > wrote:
> > > > > On Thu, Dec 18, 2008 at 7:52 AM, Jordan Crouse
> >
> > <[e-mail]
> > > > > wrote:
> > > > > > Myles Watson wrote:
> > > > > >
> > > > > > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > > > > > [e-mail]> wrote:
> > > > > > >
> > > > > > >
> > > > > > >               
> > > > > > >> Bao, Zheng found a bug which killed SATA booting on my board.
> > > > > > >>
> > > > > > >> This happened because we do not error out on implicit function
> > > > > > >> declarations. The linker has no way of checking whether the
> > > > > > >>
> >
> > implicitly
> > > > > > >> assumed function signature is identical to the real signature, so
> > > > > > >> mismatches can occur and these mismatches are practically
> > > > > > >>
> >
> > impossible
> > > > > > >> to debug because the code looks completely correct.
> > > > > > >>
> > > > > > >> Adding -Werror-implicit-function-declaration to our CFLAGS would
> > > > > > >>
> >
> > solve
> > > > > > >> this problem nicely, but a lot of files in the tree need to be
> > > > > > >>
> >
> > fixed.
> > > > > > >>                 
> > > > > > > I think this is a great idea.  Isn't the correct order to fix all
> > > > > > >
> >
> > the
> > > > > > > warnings, then make it an error?
> > > > > > >
> > > > > > >               
> > > > > > Yeah - the unfortunate thing about changes like this is that you end
> >
> > up
> > > > > > being responsible for fixing the errors.. :)
> > > > > >
> > > > > > Carl-Daniel - if you post a list of offending files, we'll all help
> > > > > > clear them up.  Dumping the log through grep "implicit
> > > > > > declaration of function" should suffice.
> > >
> > > I'm attaching a patch which I thought was trivial, but breaks things all
> > > over the place.  I'm not sure how this will need to be done.  It looks
> >
> > like
> > > a problem with romcc vs. CAR.
> >
> > Try this patch. It passes abuild.
> >
> > Regards,
> > Carl-Daniel
> >
> > Fix implicit declarations of pci_read_config8 and pci_write_config8 in
> > the following files:
> > src/mainboard/intel/jarrell/reset.c
> > src/mainboard/supermicro/x6dai_g/reset.c
> > src/mainboard/supermicro/x6dhe_g2/reset.c
> > src/mainboard/supermicro/x6dhe_g/reset.c
> > src/mainboard/supermicro/x6dhr_ig2/reset.c
> > src/mainboard/supermicro/x6dhr_ig/reset.c
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Myles Watson

Thanks, r3832.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 18.12.2008 15:52, Jordan Crouse wrote:
> Myles Watson wrote:
> > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > [e-mail]> wrote:
> > > Bao, Zheng found a bug which killed SATA booting on my board.
> > >
> > > This happened because we do not error out on implicit function
> > > declarations. The linker has no way of checking whether the implicitly
> > > assumed function signature is identical to the real signature, so
> > > mismatches can occur and these mismatches are practically impossible to
> > > debug because the code looks completely correct.
> > >
> > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > this problem nicely, but a lot of files in the tree need to be fixed.
> >
> > I think this is a great idea.  Isn't the correct order to fix all the
> > warnings, then make it an error?
>
> Yeah - the unfortunate thing about changes like this is that you end
> up being responsible for fixing the errors.. :)
>
> Carl-Daniel - if you post a list of offending files, we'll all help
> clear them up.  Dumping the log through grep "implicit declaration of
> function" should suffice.

This is the list of unfixed problems in my tree after my other patches
are applied:

src/cpu/amd/dualcore/dualcore.c:63: warning: implicit declaration of
function 'get_nodes'
src/include/cpu/x86/bist.h:13: warning: implicit declaration of function
'die'
src/include/cpu/x86/bist.h:8: warning: implicit declaration of function
'printk_emerg'
src/northbridge/amd/amdfam10/../amdmct/mct/mctndi_d.c:144: warning:
overflow in implicit constant conversio'
src/northbridge/amd/amdfam10/reset_test.c:36: warning: implicit
declaration of function 'NODE_PCI'
src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c:27: warning:
implicit declaration of function 'set_ht_link_buffer_counts_chain'
src/cpu/amd/dualcore/dualcore.c:63: warning: implicit declaration of
function 'get_nodes'
src/cpu/amd/model_fxx/init_cpus.c:319: warning: implicit declaration of
function 'soft_reset'
src/mainboard/digitallogic/msm800sev/cache_as_ram_auto.c:117: warning:
implicit declaration of function 'done_cache_as_ram_main'
src/mainboard/emulation/qemu-x86/mainboard.c:15: warning: implicit
declaration of function 'setup_realmode_idt'
src/mainboard/emulation/qemu-x86/mainboard.c:17: warning: implicit
declaration of function 'do_vgabios'
src/mainboard/emulation/qemu-x86/mainboard.c:21: warning: implicit
declaration of function 'init_pc_keyboard'
src/mainboard/intel/xe7501devkit/reset.c:4: warning: implicit
declaration of function 'i82801ca_hard_reset'
src/mainboard/pcengines/alix1c/cache_as_ram_auto.c:209: warning:
implicit declaration of function 'done_cache_as_ram_main'
src/mainboard/tyan/s2735/reset.c:4: warning: implicit declaration of
function 'i82801er_hard_reset'
src/mainboard/via/epia-m/mainboard.c:19: warning: implicit declaration
of function 'setup_realmode_idt'
src/mainboard/via/epia-m/mainboard.c:21: warning: implicit declaration
of function 'do_vgabios'
src/mainboard/via/epia-m/vgabios.c:367: warning: implicit declaration of
function 'write_protect_vgabios'
src/northbridge/amd/amdk8/northbridge.c:1174: warning: implicit
declaration of function 'read_nb_cfg_54'
src/northbridge/amd/amdk8/northbridge.c:1242: warning: implicit
declaration of function 'is_e0_later_in_bsp'
src/northbridge/amd/amdk8/northbridge.c:1244: warning: implicit
declaration of function 'is_cpu_f0_in_bsp'
src/northbridge/amd/amdk8/raminit.c:2122: warning: implicit declaration
of function 'activate_spd_rom'
src/northbridge/amd/amdk8/raminit_f.c:2789: warning: implicit
declaration of function 'activate_spd_rom'
src/northbridge/intel/e7501/northbridge.c:163: warning: implicit
declaration of function 'initialize_cpus'
src/northbridge/intel/i82830/northbridge.c:166: warning: implicit
declaration of function 'initialize_cpus'
src/northbridge/intel/i855pm/northbridge.c:130: warning: implicit
declaration of function 'initialize_cpus'
src/northbridge/via/vt8601/northbridge.c:165: warning: implicit
declaration of function 'initialize_cpus'
src/northbridge/via/vt8623/northbridge.c:163: warning: implicit
declaration of function 'setup_realmode_idt'
src/northbridge/via/vt8623/northbridge.c:165: warning: implicit
declaration of function 'do_vgabios'
src/northbridge/via/vt8623/northbridge.c:174: warning: implicit
declaration of function 'vga_enable_console'
src/southbridge/intel/i82801gx/i82801gx_lpc.c:277: warning: implicit
declaration of function 'setup_i8259'
src/southbridge/via/vt8235/vt8235.c:76: warning: implicit declaration of
function 'setup_i8259'

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 18.12.2008 15:52, Jordan Crouse wrote:
> Myles Watson wrote:
> > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > [e-mail]> wrote:
> > > Bao, Zheng found a bug which killed SATA booting on my board.
> > >
> > > This happened because we do not error out on implicit function
> > > declarations. The linker has no way of checking whether the implicitly
> > > assumed function signature is identical to the real signature, so
> > > mismatches can occur and these mismatches are practically impossible to
> > > debug because the code looks completely correct.
> > >
> > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > this problem nicely, but a lot of files in the tree need to be fixed.
> >
> > I think this is a great idea.  Isn't the correct order to fix all the
> > warnings, then make it an error?
>
> Yeah - the unfortunate thing about changes like this is that you end
> up being responsible for fixing the errors.. :)

Fix implicit declarations of get_bus_conf.

Signed-off-by: Carl-Daniel Hailfinger

abuild tested.

--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/h8dmr/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/h8dmr/mptable.c	(Arbeitskopie)
@@ -33,6 +33,8 @@

extern unsigned char bus_pcix[3]; // under bus_mcp55_2

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
         static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/h8dmr/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/supermicro/h8dmr/irq_tables.c	(Arbeitskopie)
@@ -53,6 +53,8 @@
 extern unsigned char bus_isa;
 extern unsigned char bus_mcp55[8]; //1

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/gigabyte/m57sli/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/gigabyte/m57sli/mptable.c	(Arbeitskopie)
@@ -33,6 +33,8 @@

extern unsigned bus_type[256];

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
         static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/gigabyte/m57sli/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/gigabyte/m57sli/irq_tables.c	(Arbeitskopie)
@@ -53,6 +53,8 @@
 extern unsigned char bus_isa;
 extern unsigned char bus_mcp55[8]; //1

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/gigabyte/ga_2761gxdk/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/gigabyte/ga_2761gxdk/irq_tables.c	(Arbeitskopie)
@@ -55,6 +55,8 @@
 extern unsigned char bus_isa;
 extern unsigned char bus_sis966[8]; //1

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912/mptable.c	(Arbeitskopie)
@@ -29,6 +29,8 @@

#include "mb_sysconf.h"

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
    static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912/irq_tables.c	(Arbeitskopie)
@@ -52,6 +52,8 @@
    pirq_info->rfu = rfu;
 }

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912_fam10/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912_fam10/mptable.c	(Arbeitskopie)
@@ -29,6 +29,8 @@

#include "mb_sysconf.h"

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
    static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912_fam10/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/tyan/s2912_fam10/irq_tables.c	(Arbeitskopie)
@@ -52,6 +52,8 @@
    pirq_info->rfu = rfu;
 }

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms9282/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms9282/mptable.c	(Arbeitskopie)
@@ -32,6 +32,8 @@

#include "mb_sysconf.h"

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
         static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms9282/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms9282/irq_tables.c	(Arbeitskopie)
@@ -55,6 +55,8 @@
         pirq_info->rfu = rfu;
 }

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms7260/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms7260/mptable.c	(Arbeitskopie)
@@ -31,6 +31,8 @@
 extern unsigned apicid_mcp55;
 extern unsigned bus_type[256];

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
    static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms7260/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/msi/ms7260/irq_tables.c	(Arbeitskopie)
@@ -49,6 +49,8 @@
 extern unsigned char bus_isa;
 extern unsigned char bus_mcp55[8];	// 1

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {
    struct irq_routing_table *pirq;
--- LinuxBIOSv2-implicit_declarations/src/mainboard/sunw/ultra40/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/sunw/ultra40/mptable.c	(Arbeitskopie)
@@ -32,6 +32,8 @@
 extern  unsigned sbdn3;
 extern  unsigned sbdnb;

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
         static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/sunw/ultra40/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/sunw/ultra40/irq_tables.c	(Arbeitskopie)
@@ -51,6 +51,8 @@
 extern  unsigned sbdn3;
 extern  unsigned sbdnb;

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

--- LinuxBIOSv2-implicit_declarations/src/mainboard/nvidia/l1_2pvv/mptable.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/nvidia/l1_2pvv/mptable.c	(Arbeitskopie)
@@ -29,6 +29,8 @@

#include "mb_sysconf.h"

+extern void get_bus_conf(void);
+
 void *smp_write_config_table(void *v)
 {
    static const char sig[4] = "PCMP";
--- LinuxBIOSv2-implicit_declarations/src/mainboard/nvidia/l1_2pvv/irq_tables.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/mainboard/nvidia/l1_2pvv/irq_tables.c	(Arbeitskopie)
@@ -52,6 +52,8 @@
    pirq_info->rfu = rfu;
 }

+extern void get_bus_conf(void);
+
 unsigned long write_pirq_routing_table(unsigned long addr)
 {

[Myles Watson]

> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> Sent: Monday, December 22, 2008 8:48 AM
> To: [e-mail]
> Cc: Myles Watson; Coreboot
> Subject: Re: [coreboot] [RFC] Error out on implicit declarations
>
> On 18.12.2008 15:52, Jordan Crouse wrote:
> > Myles Watson wrote:
> > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > [e-mail]> wrote:
> > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > >
> > > > This happened because we do not error out on implicit function
> > > > declarations. The linker has no way of checking whether the implicitly
> > > > assumed function signature is identical to the real signature, so
> > > > mismatches can occur and these mismatches are practically impossible
>
> to
> > > > debug because the code looks completely correct.
> > > >
> > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > this problem nicely, but a lot of files in the tree need to be fixed.
> > >
> > > I think this is a great idea.  Isn't the correct order to fix all the
> > > warnings, then make it an error?
> >
> > Yeah - the unfortunate thing about changes like this is that you end
> > up being responsible for fixing the errors.. :)
>
> Fix implicit declarations of get_bus_conf.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Myles Watson

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 22.12.2008 17:32, Myles Watson wrote:
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> > Sent: Monday, December 22, 2008 8:48 AM
> > To: [e-mail]
> > Cc: Myles Watson; Coreboot
> > Subject: Re: [coreboot] [RFC] Error out on implicit declarations
> >
> > On 18.12.2008 15:52, Jordan Crouse wrote:
> > > Myles Watson wrote:
> > > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > > [e-mail]> wrote:
> > > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > > >
> > > > > This happened because we do not error out on implicit function
> > > > > declarations. The linker has no way of checking whether the implicitly
> > > > > assumed function signature is identical to the real signature, so
> > > > > mismatches can occur and these mismatches are practically impossible
> >
> > to
> > > > > debug because the code looks completely correct.
> > > > >
> > > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > > this problem nicely, but a lot of files in the tree need to be fixed.
> > > >
> > > > I think this is a great idea.  Isn't the correct order to fix all the
> > > > warnings, then make it an error?
> > >
> > > Yeah - the unfortunate thing about changes like this is that you end
> > > up being responsible for fixing the errors.. :)
> >
> > Fix implicit declarations of get_bus_conf.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Myles Watson

Thanks, r3835.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 18.12.2008 15:52, Jordan Crouse wrote:
> Myles Watson wrote:
> > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > [e-mail]> wrote:
> > > Bao, Zheng found a bug which killed SATA booting on my board.
> > >
> > > This happened because we do not error out on implicit function
> > > declarations. The linker has no way of checking whether the implicitly
> > > assumed function signature is identical to the real signature, so
> > > mismatches can occur and these mismatches are practically impossible to
> > > debug because the code looks completely correct.
> > >
> > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > this problem nicely, but a lot of files in the tree need to be fixed.

Fix implicit udelay src/southbridge/nvidia/mcp55/mcp55_aza.c
Fix imlicit mdelay in src/southbridge/nvidia/mcp55/mcp55_nic.c

Signed-off-by: Carl-Daniel Hailfinger

abuild tested on all MCP55 targets.

Index: LinuxBIOSv2-implicit_declarations/src/southbridge/nvidia/mcp55/mcp55_nic.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/southbridge/nvidia/mcp55/mcp55_nic.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/southbridge/nvidia/mcp55/mcp55_nic.c	(Arbeitskopie)
@@ -28,6 +28,7 @@
 #include <device/pci_ids.h>
 #include <device/pci_ops.h>
 #include <arch/io.h>
+#include <delay.h>
 #include "mcp55.h"

static int phy_read(uint8_t *base, unsigned phy_addr, unsigned phy_reg)
Index: LinuxBIOSv2-implicit_declarations/src/southbridge/nvidia/mcp55/mcp55_aza.c
===================================================================
--- LinuxBIOSv2-implicit_declarations/src/southbridge/nvidia/mcp55/mcp55_aza.c	(Revision 3829)
+++ LinuxBIOSv2-implicit_declarations/src/southbridge/nvidia/mcp55/mcp55_aza.c	(Arbeitskopie)
@@ -27,6 +27,7 @@
 #include <device/pci_ids.h>
 #include <device/pci_ops.h>
 #include <arch/io.h>
+#include <delay.h>
 #include "mcp55.h"

static int set_bits(uint8_t *port, uint32_t mask, uint32_t val)

[Myles Watson]

> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> Sent: Monday, December 22, 2008 8:52 AM
> To: [e-mail]
> Cc: Myles Watson; Coreboot
> Subject: Re: [coreboot] [RFC] Error out on implicit declarations
>
> On 18.12.2008 15:52, Jordan Crouse wrote:
> > Myles Watson wrote:
> > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > [e-mail]> wrote:
> > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > >
> > > > This happened because we do not error out on implicit function
> > > > declarations. The linker has no way of checking whether the implicitly
> > > > assumed function signature is identical to the real signature, so
> > > > mismatches can occur and these mismatches are practically impossible
>
> to
> > > > debug because the code looks completely correct.
> > > >
> > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > this problem nicely, but a lot of files in the tree need to be fixed.
>
> Fix implicit udelay src/southbridge/nvidia/mcp55/mcp55_aza.c
> Fix imlicit mdelay in src/southbridge/nvidia/mcp55/mcp55_nic.c
>
> Signed-off-by: Carl-Daniel Hailfinger
>
> abuild tested on all MCP55 targets.

Acked-by: Myles Watson

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 22.12.2008 17:01, Myles Watson wrote:
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> > Sent: Monday, December 22, 2008 8:52 AM
> > To: [e-mail]
> > Cc: Myles Watson; Coreboot
> > Subject: Re: [coreboot] [RFC] Error out on implicit declarations
> >
> > On 18.12.2008 15:52, Jordan Crouse wrote:
> > > Myles Watson wrote:
> > > > On Thu, Dec 18, 2008 at 3:18 AM, Carl-Daniel Hailfinger <
> > > > [e-mail]> wrote:
> > > > > Bao, Zheng found a bug which killed SATA booting on my board.
> > > > >
> > > > > This happened because we do not error out on implicit function
> > > > > declarations. The linker has no way of checking whether the implicitly
> > > > > assumed function signature is identical to the real signature, so
> > > > > mismatches can occur and these mismatches are practically impossible
> >
> > to
> > > > > debug because the code looks completely correct.
> > > > >
> > > > > Adding -Werror-implicit-function-declaration to our CFLAGS would solve
> > > > > this problem nicely, but a lot of files in the tree need to be fixed.
> >
> > Fix implicit udelay src/southbridge/nvidia/mcp55/mcp55_aza.c
> > Fix imlicit mdelay in src/southbridge/nvidia/mcp55/mcp55_nic.c
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> >
> > abuild tested on all MCP55 targets.
>
> Acked-by: Myles Watson

Thanks, r3831.

Regards,
Carl-Daniel

[ron minnich]

btw, I think a writeup of how this broke would be extremely valuable
in an historical/tutorial sense. What happened that an implicit went
wrong?

ron