Patchwork [U-Boot,01/39] DEBUG: Fix debug macros

login
register
mail settings
Submitter Marek Vasut
Date Oct. 22, 2011, 12:16 a.m.
Message ID <1319242654-15534-2-git-send-email-marek.vasut@gmail.com>
Download mbox | patch
Permalink /patch/121098/
State Superseded
Headers show

Comments

Marek Vasut - Oct. 22, 2011, 12:16 a.m.
The current implementation of debug doesn't play well with GCC4.6.
This implementation also fixes GCC4.6 complaints about unused variables
while maintaining code size.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 include/common.h |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)
Mike Frysinger - Oct. 23, 2011, 5:37 a.m.
On Fri, Oct 21, 2011 at 20:16, Marek Vasut wrote:
> The current implementation of debug doesn't play well with GCC4.6.
> This implementation also fixes GCC4.6 complaints about unused variables
> while maintaining code size.

i think this patch should come last rather than first that way you fix
all the warnings/failures *before* making the debug() code
unconditional

> +#define debug_cond(cond, fmt, args...)         \
> +       do {                                    \
> +               if (cond)                       \
> +                       printf(fmt, ##args);    \
> +       } while (0)
> +
> +#define debug(fmt, args...)                    \
> +       debug_cond(_DEBUG, fmt, ##args)
> +
> +#define debugX(level, fmt, args...)            \
> +       debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args)

i thought we were just going to punt debugX() ?
-mike
Wolfgang Denk - Oct. 23, 2011, 5:12 p.m.
Dear Mike Frysinger,

In message <CAJaTeTpwpO6Kd-q=bmbzqPJMoNz_YiOWGRks=zGkhes0vUA=mg@mail.gmail.com> you wrote:
>
> > +#define debugX(level, fmt, args...)            \
> > +       debug_cond((_DEBUG && DEBUG >=3D (level)), fmt, ##args)
> 
> i thought we were just going to punt debugX() ?

Yes, please do!

Best regards,

Wolfgang Denk
Simon Glass - Oct. 24, 2011, 4:31 a.m.
Hi Marek,

On Fri, Oct 21, 2011 at 5:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> The current implementation of debug doesn't play well with GCC4.6.
> This implementation also fixes GCC4.6 complaints about unused variables
> while maintaining code size.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  include/common.h |   20 ++++++++++++--------
>  1 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/common.h b/include/common.h
> index eb19a44..c3b23551 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -116,20 +116,24 @@ typedef volatile unsigned char    vu_char;
>  #include <flash.h>
>  #include <image.h>
>
> -#ifdef DEBUG
> -#define debug(fmt,args...)     printf (fmt ,##args)
> -#define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args);
> -#else
> -#define debug(fmt,args...)
> -#define debugX(level,fmt,args...)
> -#endif /* DEBUG */
> -
>  #ifdef DEBUG
>  # define _DEBUG 1
>  #else
>  # define _DEBUG 0
>  #endif
>
> +#define debug_cond(cond, fmt, args...)         \

Yes this is much nicer. Could perhaps add a little comment about how
to use this and to avoid putting debug() inside #ifdef?

> +       do {                                    \
> +               if (cond)                       \
> +                       printf(fmt, ##args);    \
> +       } while (0)
> +
> +#define debug(fmt, args...)                    \
> +       debug_cond(_DEBUG, fmt, ##args)
> +
> +#define debugX(level, fmt, args...)            \
> +       debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args)
> +
>  /*
>  * An assertion is run-time check done in debug mode only. If DEBUG is not
>  * defined then it is skipped. If DEBUG is defined and the assertion fails,
> --
> 1.7.6.3
>
>
Marek Vasut - Oct. 24, 2011, 8:21 a.m.
On Monday, October 24, 2011 06:31:19 AM Simon Glass wrote:
> Hi Marek,
> 
> On Fri, Oct 21, 2011 at 5:16 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > The current implementation of debug doesn't play well with GCC4.6.
> > This implementation also fixes GCC4.6 complaints about unused variables
> > while maintaining code size.
> > 
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> >  include/common.h |   20 ++++++++++++--------
> >  1 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/common.h b/include/common.h
> > index eb19a44..c3b23551 100644
> > --- a/include/common.h
> > +++ b/include/common.h
> > @@ -116,20 +116,24 @@ typedef volatile unsigned char    vu_char;
> >  #include <flash.h>
> >  #include <image.h>
> > 
> > -#ifdef DEBUG
> > -#define debug(fmt,args...)     printf (fmt ,##args)
> > -#define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args);
> > -#else
> > -#define debug(fmt,args...)
> > -#define debugX(level,fmt,args...)
> > -#endif /* DEBUG */
> > -
> >  #ifdef DEBUG
> >  # define _DEBUG 1
> >  #else
> >  # define _DEBUG 0
> >  #endif
> > 
> > +#define debug_cond(cond, fmt, args...)         \
> 
> Yes this is much nicer. Could perhaps add a little comment about how
> to use this and to avoid putting debug() inside #ifdef?
> 
> > +       do {                                    \
> > +               if (cond)                       \
> > +                       printf(fmt, ##args);    \
> > +       } while (0)
> > +
> > +#define debug(fmt, args...)                    \
> > +       debug_cond(_DEBUG, fmt, ##args)
> > +
> > +#define debugX(level, fmt, args...)            \
> > +       debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args)
> > +
> >  /*
> >  * An assertion is run-time check done in debug mode only. If DEBUG is
> > not * defined then it is skipped. If DEBUG is defined and the assertion
> > fails, --
> > 1.7.6.3

Done, I pushed new patchset right now to git://git.denx.de/u-boot-marex.git , 
"debug" branch. It's around 50 patches now.
Wolfgang Denk - Oct. 24, 2011, 7:21 p.m.
Dear Marek Vasut,

In message <201110241021.15442.marek.vasut@gmail.com> you wrote:
>
...
> Done, I pushed new patchset right now to git://git.denx.de/u-boot-marex.git , 
> "debug" branch. It's around 50 patches now.

Keep in mind that nobody cares about patches anywhere in any
repository.  What has not been sent to the mailing list and thus is
captured in the list archives and patchwork simply does not exist.

Best regards,

Wolfgang Denk
Marek Vasut - Oct. 25, 2011, 12:11 a.m.
> Dear Marek Vasut,
> 
> In message <201110241021.15442.marek.vasut@gmail.com> you wrote:
> 
> ...
> 
> > Done, I pushed new patchset right now to
> > git://git.denx.de/u-boot-marex.git , "debug" branch. It's around 50
> > patches now.
> 
> Keep in mind that nobody cares about patches anywhere in any
> repository.  What has not been sent to the mailing list and thus is
> captured in the list archives and patchwork simply does not exist.

Hi Wolfgang,

I know this very well, I just need to feedback and the patchset is really huge. 
I'd wait for you to merge some of the stuff from this patchset (you can merge 
what you feel fitting but the 0001 patch ...) and then rebase and resubmit the 
rest on top of that.

Cheers
Wolfgang Denk - Oct. 25, 2011, 7:37 a.m.
Dear Marek Vasut,

In message <201110250211.07058.marek.vasut@gmail.com> you wrote:
>
> I know this very well, I just need to feedback and the patchset is really huge. 

What you attempt will not work.

You may get some feedback on patches posted on the list.

I will_never_ send any feedback to any stuff in some repository
anywhere. I will never even try to read it.  And I m pretty sure many
others work like that, too.

If the patch set is huge, then do yourself a favour and break it down
into small, managable pieces.   Don't try to do everything at once.

Send a short sub-set first - maybe some 4...8 patches or so.  Maybe
even 10.  Then let people digest these, and only when they are
scheduled for merging start and post the next of your batches.

_Nobody_ will review a 50 patch series.  And nobdy will apply it
either.

> I'd wait for you to merge some of the stuff from this patchset (you can merge 
> what you feel fitting but the 0001 patch ...) and then rebase and resubmit the 
> rest on top of that.

I'd rather you split the series in smaller, digestable batches.

Best regards,

Wolfgang Denk
Marek Vasut - Oct. 25, 2011, 8:27 a.m.
> Dear Marek Vasut,
> 
> In message <201110250211.07058.marek.vasut@gmail.com> you wrote:
> > I know this very well, I just need to feedback and the patchset is really
> > huge.
> 
> What you attempt will not work.
> 
> You may get some feedback on patches posted on the list.
> 
> I will_never_ send any feedback to any stuff in some repository
> anywhere. I will never even try to read it.  And I m pretty sure many
> others work like that, too.

I expect that, all right.
> 
> If the patch set is huge, then do yourself a favour and break it down
> into small, managable pieces.   Don't try to do everything at once.

Well I'm not quite sure how you'd like to break a set of patches like this one 
into smaller chunks ... ?
> 
> Send a short sub-set first - maybe some 4...8 patches or so.  Maybe
> even 10.  Then let people digest these, and only when they are
> scheduled for merging start and post the next of your batches.

Well ... it's insane. Can you start picking patches from this one series which 
you feel are ok? I'll rebase the rest on that and submit it. The patches are 
pretty much independent anyway.
> 
> _Nobody_ will review a 50 patch series.  And nobdy will apply it
> either.
> 
> > I'd wait for you to merge some of the stuff from this patchset (you can
> > merge what you feel fitting but the 0001 patch ...) and then rebase and
> > resubmit the rest on top of that.
> 
> I'd rather you split the series in smaller, digestable batches.

Well see above. Can you cherrypick something from this series already which you 
feel fitting ? There's no dependency anyway.

> 
> Best regards,
> 
> Wolfgang Denk

Cheers
Graeme Russ - Oct. 25, 2011, 8:49 a.m.
Hi Marek,

On 25/10/11 19:27, Marek Vasut wrote:
>> Dear Marek Vasut,
>>
>> In message <201110250211.07058.marek.vasut@gmail.com> you wrote:
>>> I know this very well, I just need to feedback and the patchset is really
>>> huge.

[snip]

>> I'd rather you split the series in smaller, digestable batches.
> 
> Well see above. Can you cherrypick something from this series already which you 
> feel fitting ? There's no dependency anyway.

Why not break it down into /common, /drivers, /arch etc - does that get it
down to less than ten per set? /drivers will probably by >10 so you can
then go /drivers/serial, /drivers/mtd until there are less than ten - put
these in a patchset called /drivers/various

Regards,

Graeme
Marek Vasut - Oct. 25, 2011, 9:03 a.m.
> Hi Marek,
> 
> On 25/10/11 19:27, Marek Vasut wrote:
> >> Dear Marek Vasut,
> >> 
> >> In message <201110250211.07058.marek.vasut@gmail.com> you wrote:
> >>> I know this very well, I just need to feedback and the patchset is
> >>> really huge.
> 
> [snip]
> 
> >> I'd rather you split the series in smaller, digestable batches.
> > 
> > Well see above. Can you cherrypick something from this series already
> > which you feel fitting ? There's no dependency anyway.
> 
> Why not break it down into /common, /drivers, /arch etc - does that get it
> down to less than ten per set? /drivers will probably by >10 so you can
> then go /drivers/serial, /drivers/mtd until there are less than ten - put
> these in a patchset called /drivers/various

This sounds good, yes. Roger that.
> 
> Regards,
> 
> Graeme
Wolfgang Denk - Oct. 25, 2011, 6:34 p.m.
Dear Marek Vasut,

In message <201110251027.10375.marek.vasut@gmail.com> you wrote:
>
> > If the patch set is huge, then do yourself a favour and break it down
> > into small, managable pieces.   Don't try to do everything at once.
> 
> Well I'm not quite sure how you'd like to break a set of patches like this one 
> into smaller chunks ... ?

$ ls | split -l 8

? :-)

> Well ... it's insane. Can you start picking patches from this one series which 
> you feel are ok? I'll rebase the rest on that and submit it. The patches are 
> pretty much independent anyway.

No, I cannot.  I don't have the time to review 50+ patches and pick
some that I "feel are ok".

> Well see above. Can you cherrypick something from this series already which you 
> feel fitting ? There's no dependency anyway.

Technically I could do that, of course.  But within the given
limitations of time and resources I will not be able to do that.

Sorry, but I ask again to provide small, managable pieces.

Best regards,

Wolfgang Denk
Joe Hershberger - Oct. 25, 2011, 8:10 p.m.
On Sun, Oct 23, 2011 at 12:12 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Mike Frysinger,
>
> In message <CAJaTeTpwpO6Kd-q=bmbzqPJMoNz_YiOWGRks=zGkhes0vUA=mg@mail.gmail.com> you wrote:
>>
>> > +#define debugX(level, fmt, args...)            \
>> > +       debug_cond((_DEBUG && DEBUG >=3D (level)), fmt, ##args)
>>
>> i thought we were just going to punt debugX() ?
>
> Yes, please do!

I'm working on a patch for the net code that benefits from the debugX
macro.  Especially for the net code, the debug traces can be
overwhelming depending on the issue you are looking at.  I recommend
we keep it around.

Thanks
-Joe
Macpaul Lin - Oct. 25, 2011, 8:40 p.m.
Hi all,

2011/10/26 Joe Hershberger <joe.hershberger@gmail.com>:
> On Sun, Oct 23, 2011 at 12:12 PM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Mike Frysinger,
>>
> I'm working on a patch for the net code that benefits from the debugX
> macro.  Especially for the net code, the debug traces can be
> overwhelming depending on the issue you are looking at.  I recommend
> we keep it around.
>
> Thanks
> -Joe

I agreed with Joe,

I feel some debug code is necessary to keep it into levels since some
you usually need to split debug level into 2 or 3 to help the debug work.
A device driver which has been commit into the mainline source should be
worked on most of platforms. However, it still might be need to do debug work
on several different platform since the hardware integration might be different
then introduce problem. Hence the debug codes in detail should not be removed
in the mainline code, and it should be labeled with some debug levels.
Those code will help the debug work in future.

Moreover, some device driver will need to be fixed when the hardware
will be fixed
or upgraded and introduce different versions. Those hardware still
follow a common
behaviors, a common set of registers, even the 50% usage of the driver
will be same.
Hence keep debug code in deep level will be help.

We can keep it until someone really have time to fix it.
Wolfgang Denk - Oct. 25, 2011, 9:59 p.m.
Dear =?UTF-8?B?6aas5YWL5rOh?=,

In message <CACCg+XP8zoKGproJ4t=QEQU4ZFYccAPPjsGkz72BKBwowcUQng@mail.gmail.com> you wrote:
>
> > I'm working on a patch for the net code that benefits from the debugX
> > macro.  Especially for the net code, the debug traces can be
> > overwhelming depending on the issue you are looking at.  I recommend
> > we keep it around.
...
> I agreed with Joe,

And I disagree.

After 11 years of U-Boot development, after collecting 2,515 C files,
we have just 2 (in words: TWO) files that use this macro.  And one of
them with a single level only, so it's even useless there.

> I feel some debug code is necessary to keep it into levels since some
> you usually need to split debug level into 2 or 3 to help the debug work.

Usually?  In a single case out of 2.5k files?  That's not usual,
that's a mistake.


I say: we get rid of this!

Best regards,

Wolfgang Denk
Joe Hershberger - Oct. 25, 2011, 10:08 p.m.
Hi Wolfgang,
On Tue, Oct 25, 2011 at 4:59 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear =?UTF-8?B?6aas5YWL5rOh?=,
>
> In message <CACCg+XP8zoKGproJ4t=QEQU4ZFYccAPPjsGkz72BKBwowcUQng@mail.gmail.com> you wrote:
>>
>> > I'm working on a patch for the net code that benefits from the debugX
>> > macro.  Especially for the net code, the debug traces can be
>> > overwhelming depending on the issue you are looking at.  I recommend
>> > we keep it around.
> ...
>> I agreed with Joe,
>
> And I disagree.

In the net code, I separated out the debug prints based on
* internal state changes
* any traffic on the network
* packets targeted to the device

I found this very helpful in debugging to limit the volume of traces
depending on what I was looking for.  How would you recommend getting
similar behavior without debugX?

Thanks,
Joe

Patch

diff --git a/include/common.h b/include/common.h
index eb19a44..c3b23551 100644
--- a/include/common.h
+++ b/include/common.h
@@ -116,20 +116,24 @@  typedef volatile unsigned char	vu_char;
 #include <flash.h>
 #include <image.h>
 
-#ifdef	DEBUG
-#define debug(fmt,args...)	printf (fmt ,##args)
-#define debugX(level,fmt,args...) if (DEBUG>=level) printf(fmt,##args);
-#else
-#define debug(fmt,args...)
-#define debugX(level,fmt,args...)
-#endif	/* DEBUG */
-
 #ifdef DEBUG
 # define _DEBUG 1
 #else
 # define _DEBUG 0
 #endif
 
+#define debug_cond(cond, fmt, args...)		\
+	do {					\
+		if (cond)			\
+			printf(fmt, ##args);	\
+	} while (0)
+
+#define debug(fmt, args...)			\
+	debug_cond(_DEBUG, fmt, ##args)
+
+#define debugX(level, fmt, args...)		\
+	debug_cond((_DEBUG && DEBUG >= (level)), fmt, ##args)
+
 /*
  * An assertion is run-time check done in debug mode only. If DEBUG is not
  * defined then it is skipped. If DEBUG is defined and the assertion fails,