Re: [coreboot] libpayload: Missing inclusion of `libpayload-config.h`

[Nico Huber]

Dear Paul,

Am 28.03.2013 12:37, schrieb Paul Menzel:
> Am Mittwoch, den 27.03.2013, 20:49 +0100 schrieb Nico Huber:
> > Am 27.03.2013 14:04, schrieb Paul Menzel:
> > > Dear coreboot folks,
> > >
> > > using latest master
> > >     commit 3cc0d1eb3f611cb7bf0e45d8ccdb0c84f54f54dc
> > >     Author: David Hendricks <[e-mail]>
> > >     Date:   Tue Mar 26 16:28:21 2013 -0700
> > >
> > >         exynos5250: assign RAM resources in cpu_init()
> > >
> > >     […]
> > >
> > >         Reviewed-on: http://review.coreboot.org/2923
> > >
> > > it looks like libpayload’s PDCurses backend is not including
> > > `libpayload-config.h` in some files so the configuration is ignored,
> > > right? I noticed this due to the following warning.
> > >     /src/coreboot/payloads/libpayload(master) $ make clean
> > >     /src/coreboot/payloads/libpayload(master) $ make
> > >     […]
> > >         CC         curses/pdcurses-backend/pdcscrn.libcurses.o
> > >     curses/pdcurses-backend/pdcscrn.c: In function 'PDC_scr_open':
> > >     curses/pdcurses-backend/pdcscrn.c:75:5: warning: "CONFIG_SPEAKER" is not defined [-Wundef]
> > >     […]
> >
> > I don't see the include missing: `pdcscrn.c` includes `lppdc.h` which,
> > in turn, includes `libpayload-config.h`.
> > > No warnings are printed for the other files, because `#ifdef` is used
> > > there. But these macros will never be defined, when
> > > `libpayload-config.h` is not included, if I am not mistaken.
> > >
> > > So is the solution to just include `config.h` or `libpayload-config.h`
> > > everywhere?
> > >     /src/coreboot/payloads/libpayload(master) $ grep -R CONFIG_SPEA .
> > >     ./configs/defconfig:CONFIG_SPEAKER=y
> > >     ./curses/pdcurses-backend/pdcscrn.c:#ifdef CONFIG_SPEAKER
> > >     ./curses/pdcurses-backend/pdcutil.c:#ifdef CONFIG_SPEAKER
> > >     ./curses/tinycurses.c:#ifdef CONFIG_SPEAKER
> > >     ./drivers/Makefile.inc:libc-$(CONFIG_SPEAKER) += speaker.c
> > >     ./build/config.h:#define CONFIG_SPEAKER 1
> > >     ./build/auto.conf:CONFIG_SPEAKER=y
> > >     ./build/libpayload-config.h:#define CONFIG_SPEAKER 1
> > >     ./.config:CONFIG_SPEAKER=y
> > >     ./tests/libpayload-config.h:#define CONFIG_SPEAKER 1
> > >
> > > If you tell me the preferred solution, I am going to submit a patch.
> >
> > Libpayload's config mechanism doesn't define unset options (unlike the one
> > coreboot uses).
>
> My problem, as seen in my paste above, is that `CONFIG_SPEAKER` is
> defined and I still get the error.

I can't reproduce the warning with CONFIG_SPEAKER being set. I've tested the
following with the reference toolchain (plus current toolchain of ArchLinux).
Starting with Stefan's patch [1] for the size_t issue:

$ git fetch http://review.coreboot.org/coreboot refs/changes/33/2933/1
Von http://review.coreboot.org/coreboot
 * branch            refs/changes/33/2933/1 -> FETCH_HEAD

$ git checkout FETCH_HEAD
[...]
Zweigspitze (HEAD) ist jetzt bei c33a40b... libpayload: drop size_t and ssize_t

$ make distclean

$ rm -f .xcompile

$ yes '' | make oldconfig
[...]

$ # check used toolchain:
$ cat .xcompile
[...]
# elf32-i386 toolchain (/home/icoN/workspace/coreboot/coreboot/payloads/libpayload/../../util/crossgcc/xgcc/bin/i386-elf-gcc)
[...]

$ # make sure CONFIG_SPEAKER is unset:
$ sed -i -e 's,CONFIG_SPEAKER=y,# CONFIG_SPEAKER is not set,' .config

$ make oldconfig
[...]

$ make build/curses/pdcurses-backend/pdcscrn.libcurses.o
    CC         curses/pdcurses-backend/pdcscrn.libcurses.o
curses/pdcurses-backend/pdcscrn.c: In function 'PDC_scr_open':
curses/pdcurses-backend/pdcscrn.c:75:5: warning: "CONFIG_SPEAKER" is not defined [-Wundef]

$ # now set CONFIG_SPEAKER:
$ sed -i -e 's,# CONFIG_SPEAKER is not set,CONFIG_SPEAKER=y,' .config

$ make oldconfig
[...]

$ grep CONFIG_SPEAKER build/libpayload-config.h
#define CONFIG_SPEAKER 1

$ make build/curses/pdcurses-backend/pdcscrn.libcurses.o
build/libpayload-config.h build/config.h differ: byte 99, line 4
    CC         curses/pdcurses-backend/pdcscrn.libcurses.o

The warning disappears with CONFIG_SPEAKER being set.

Regards,
Nico

[1] http://review.coreboot.org/2933