[PATCH] Debugging facility improvements

[Uwe Hermann]

See patch.

Uwe.

[Peter Stuge]

Uwe Hermann wrote:
> Debugging facility improvements.
>
> Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.
>
> Get rid of a custom "debug" macro, use printk() as usual.
>
> Signed-off-by: Uwe Hermann
> Index: src/Kconfig
>
> --- src/Kconfig	(Revision 5940)
> +++ src/Kconfig	(Arbeitskopie)
> @@ -553,6 +553,16 @@
>  If unsure, say N.
>
> +config DEBUG_MALLOC
> bool "Output verbose malloc debug messages"
> default n
> help
>  This option enables additional malloc related debug messages.
>  Note: This option will increase the size of the coreboot image.
>  If unsure, say N.

Should this also force debug level to SPEW?
> +++ src/lib/malloc.c	(Arbeitskopie)
> @@ -1,11 +1,12 @@
>  #include <stdlib.h>
>  #include <console/console.h>
>
> -#if 0
> +#if CONFIG_DEBUG_MALLOC
> +#define MALLOCDBG(x...) printk(BIOS_SPEW, x)
> +#else

So that messages actually always show up when the option is selected?

//Peter

[Uwe Hermann]

On Tue, Oct 12, 2010 at 09:46:47AM +0200, Peter Stuge wrote:
> Uwe Hermann wrote:
> > Debugging facility improvements.
> >
> > Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.
> >
> > Get rid of a custom "debug" macro, use printk() as usual.
> >
> > Signed-off-by: Uwe Hermann
> > Index: src/Kconfig
> >
> > --- src/Kconfig	(Revision 5940)
> > +++ src/Kconfig	(Arbeitskopie)
> > @@ -553,6 +553,16 @@
> >  If unsure, say N.
> >
> > +config DEBUG_MALLOC
> > bool "Output verbose malloc debug messages"
> > default n
> > help
> >  This option enables additional malloc related debug messages.
> >  Note: This option will increase the size of the coreboot image.
> >  If unsure, say N.
>
> Should this also force debug level to SPEW?

No idea, but it's unrelated to this patch. As far as I can see all other
such DEBUG_* mechanisms don't force the debug level to SPEW either, so
if we want to do that (I'm not sure we do) that's material for another
patch.

Uwe.

[Peter Stuge]

Uwe Hermann wrote:
> > > Debugging facility improvements.
> > > Hook up malloc() debug code via CONFIG_DEBUG_MALLOC.

..
> > > +++ src/Kconfig	(Arbeitskopie)
> > > @@ -553,6 +553,16 @@
> > >  If unsure, say N.
> > >
> > > +config DEBUG_MALLOC
> > > bool "Output verbose malloc debug messages"
> > > default n
> > > help
> > >  This option enables additional malloc related debug messages.
> > >  Note: This option will increase the size of the coreboot image.
> > >  If unsure, say N.
> >
> > Should this also force debug level to SPEW?
>
> No idea, but it's unrelated to this patch.

I disagree.
> As far as I can see all other such DEBUG_* mechanisms don't force
> the debug level to SPEW either, so if we want to do that (I'm not
> sure we do) that's material for another patch.

My point is that the patch claims to improve debugging, but in fact
it adds the possibility that a user creates a configuration where
they expect malloc debug messages but in fact do not get any because
there is a glitch in Kconfig between the option they enabled and the
requirement in code for SPEW loglevel for that option to have any
effect. I don't think we should accept glitches like that, it is
potentially a huge waste of time for the user, as well as extremely
annoying. It makes coreboot look really unprofessional. :\

//Peter

[Uwe Hermann]

On Tue, Oct 26, 2010 at 07:19:59AM +0200, Peter Stuge wrote:
> > > Should this also force debug level to SPEW?
> >
> > No idea, but it's unrelated to this patch.
>
> I disagree.

We agree to disagree :)
> > As far as I can see all other such DEBUG_* mechanisms don't force
> > the debug level to SPEW either, so if we want to do that (I'm not
> > sure we do) that's material for another patch.
>
> My point is that the patch claims to improve debugging, but in fact
> it adds the possibility that a user creates a configuration where
> they expect malloc debug messages but in fact do not get any because
> there is a glitch in Kconfig between the option they enabled and the
> requirement in code for SPEW loglevel for that option to have any
> effect. I don't think we should accept glitches like that, it is
> potentially a huge waste of time for the user, as well as extremely
> annoying. It makes coreboot look really unprofessional. :\

Yep. Updated patch as discussed on IRC. We only show the various DEBUG
options if at least DEBUG or SPEW was selected as loglevel in
menuconfig, as otherwise the additional debug code would not be printed
anyway.

Uwe.

[Peter Stuge]

Uwe Hermann wrote:
> Debugging facility improvements.
>
> Hook up malloc() debug code via CONFIG_DEBUG_MALLOC. Only show it in
> menuconfig if at least DEBUG or SPEW are selected as loglevel, as this
> code does additional printk(BIOS_DEBUG, ...) calls which would otherwise
> not be visible anyway.
>
> Similarly, make DEBUG_CAR and REALMODE_DEBUG only visible if thr DEBUG or
> SPEW loglevel is selected.
>
> Get rid of a custom "debug" macro, use printk() as usual.
>
> Signed-off-by: Uwe Hermann

Acked-by: Peter Stuge

[Uwe Hermann]

On Wed, Nov 10, 2010 at 12:35:40AM +0100, Peter Stuge wrote:
> Acked-by: Peter Stuge

Thanks, r6054.

Uwe.

[Warren Turkal]

Does the following part of the help text add any value:
"Note: This option will increase the size of the coreboot image."

I would think that it would be obvious that additional debug functionality 
makes the coreboot image bigger. Could you just strip that from the help text?

Also, posting patches in the body of your messages might make it easier for 
reviewers to comment on specific lines. Would it be possible for you to do so 
in the future for smaller patches like this one?

Thanks,
wt