Re: [PATCH] drm/i915: Introduce i915_dbg macro

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ke, 2016-01-27 at 17:32 +0000, Dave Gordon wrote:
> On 26/01/16 09:44, Joonas Lahtinen wrote:
> > On ma, 2016-01-25 at 18:57 +0000, Dave Gordon wrote:
> > > On 25/01/16 18:17, Daniel Vetter wrote:
> > > 

<SNIP>

> > > 
> > > I915_DBG(...) ?
> > > 
> > > It's conventional that macros should be UPPERCASE.
> > > 
> > > Especially when some config options may mean that the code
> > > disappears
> > > entirely, so you have to be sure not to use arguments with side-
> > > effects!
> > 
> > Slight correction here (for future), from Kernel Coding Style
> > documentation;
> > 
> > "CAPITALIZED macro names are appreciated but macros resembling
> > functions may be named in lower case."
> > 
> > And looking at "include/linux/device.h", dev_dbg definition is a
> > macro
> > too, like almost all the printing functions. I'd rather see it as
> > i915_dbg. Arguments with side effects can be handled nicely as can
> > be
> > seen.
> > 
> > We really should increase the priority of modernizing the debugging
> > infrastructure for i915 (and as a dependency for DRM as Daniel
> > hoped).
> > 
> > Regards, Joonas
> > 
> > > .Dave.
> 
> The fact that the upstream definitions are not great doesn't mean we 
> should copy the flaws:
> 

You missed my whole point, "dev_dbg definition is a macro too, like
almost all the printing functions. I'd rather see it as i915_dbg."
Probably should have written "as i915_dbg than I915_DBG". No matter how
the implementation, the name should be consistent with dev_dbg.

> #if defined(CONFIG_DYNAMIC_DEBUG)
> #define dev_dbg(dev, format, ...)                    \
> do {                                                 \
>          dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
> } while (0)
> #elif defined(DEBUG)
> #define dev_dbg(dev, format, arg...)            \
>          dev_printk(KERN_DEBUG, dev, format, ##arg)
> #else
> #define dev_dbg(dev, format, arg...)                            \
> ({                                                              \
>          if (0)                                                  \
>                  dev_printk(KERN_DEBUG, dev, format, ##arg);     \
> })
> #endif
> 
> So what's wrong with the above?
> 
> Firstly, the CONFIG_DYNAMIC_DEBUG version is wrapped in a do-while(0) 
> but the others aren't; this makes them different syntactically - it's a 
> statement body, whereas the others are (void) expressions. In either 
> case, writing
> 	x = dev_dbg(...);
> will give an error (different errors, though!). But the following:
> 	x = 1, dev_dbg(...);
> compiles if not CONFIG_DYNAMIC_DEBUG. You probably wouldn't write the 
> above, but it could itself be the result of a macro expansion, and it 
> would work (x is assigned 1, dev_dbg() is called) ... until you try to 
> enable dynamic debug.
> 
> (IMHO they should all be wrapped, which ensures you can't get away with 
> using it in any other way than as a statement.)
> 
> Secondly, the CONFIG_DYNAMIC_DEBUG version uses the C99 __VA_ARGS__ 
> syntax, whereas the others use the GCC-specific "arg..." method. This 
> *probably* won't matter but it's an unnecessary inconsistency.
> 
> Thirdly, the non-DEBUG version doesn't evaluate its arguments, whereas 
> the other two obviously do. So code that includes a side-effect inside 
> the parameters to the call will behave differently; and there'll be no 
> clue at all that something that looks like a regular function call:
> 
> 	dev_dbg(mydev, "Been here %d times now", ++i);
> 
> ... may or may not increment i, depending on the compile-time definition 
> above.

That actually seems to happen;

$ git grep dev_dbg | grep "++" | wc -l
16

> This is just laying traps for the developer; calling it DEV_DBG() 
> might at least make people *notice* that it's a macro not a function!
> 

Meh, just improve the existing stuff. Seems like you did a good
analysis on the current code, why not make some patches to fix it and
other similar code?

CONFIG_DYNAMIC_DEBUG code appeared afterwards, that's probably one
reason for the current state. While you're at it, #ifdef logic
for CONFIG_DYNAMIC_DEBUG, DEBUG and no-DEBUG is having different
preference in different places (DEBUG or CONFIG_DYNAMIC_DEBUG to
dominate).

Regards, Joonas

> .Dave.
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx