| Message ID | 1450435157-17318-3-git-send-email-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 18, 2015 at 12:39:17PM +0200, Joonas Lahtinen wrote: > Using __stringify(x) instead of #x adds support for macros as > a parameter and reduces runtime overhead. > > Slightly increases the .text size but should not matter. > > v2: > - Define I915_STATE_WARN_ON though I915_STATE_WARN > (Bikeshed inspiration by Chris) > > Cc: Rob Clark <robdclark@gmail.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Neat. Both Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 18/12/15 10:39, Joonas Lahtinen wrote: > Using __stringify(x) instead of #x adds support for macros as > a parameter and reduces runtime overhead. > > Slightly increases the .text size but should not matter. > > v2: > - Define I915_STATE_WARN_ON though I915_STATE_WARN > (Bikeshed inspiration by Chris) > > Cc: Rob Clark <robdclark@gmail.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5a5a3e0..1ccd137 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -69,11 +69,11 @@ > BUILD_BUG_ON(__i915_warn_cond); \ > WARN(__i915_warn_cond, "WARN_ON(" #x ")"); }) > #else > -#define WARN_ON(x) WARN((x), "WARN_ON(%s)", #x ) > +#define WARN_ON(x) WARN((x), "WARN_ON(" __stringify(x) ")") > #endif > > #undef WARN_ON_ONCE > -#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(%s)", #x ) > +#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(" __stringify(x) ")") > > #define MISSING_CASE(x) WARN(1, "Missing switch case (%lu) in %s\n", \ > (long) (x), __func__); > @@ -93,14 +93,8 @@ > unlikely(__ret_warn_on); \ > }) > > -#define I915_STATE_WARN_ON(condition) ({ \ > - int __ret_warn_on = !!(condition); \ > - if (unlikely(__ret_warn_on)) \ > - if (!WARN(i915.verbose_state_checks, \ > - "WARN_ON(" #condition ")\n")) \ > - DRM_ERROR("WARN_ON(" #condition ")\n"); \ > - unlikely(__ret_warn_on); \ > -}) > +#define I915_STATE_WARN_ON(x) \ > + I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")") > > static inline const char *yesno(bool v) > { NAK. This will give compile-time warnings for lines such as: WARN_ON(x%16 != 0); because the stringified text of the expression (which in this case contains a "%" character) would appear as part of the format string, rather than inside an argument. See: 4eee492 drm/i915: fix driver's versions of WARN_ON & WARN_ON_ONCE .Dave.
On pe, 2015-12-18 at 12:03 +0000, Dave Gordon wrote: > On 18/12/15 10:39, Joonas Lahtinen wrote: > > Using __stringify(x) instead of #x adds support for macros as > > a parameter and reduces runtime overhead. > > > > Slightly increases the .text size but should not matter. > > > > v2: > > - Define I915_STATE_WARN_ON though I915_STATE_WARN > > (Bikeshed inspiration by Chris) > > > > Cc: Rob Clark <robdclark@gmail.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > <SNIP> > > > > \ > > -}) > > +#define I915_STATE_WARN_ON(x) > > \ > > + I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")") > > > > static inline const char *yesno(bool v) > > { > > NAK. > > This will give compile-time warnings for lines such as: > > WARN_ON(x%16 != 0); > > because the stringified text of the expression (which in this case > contains a "%" character) would appear as part of the format string, > rather than inside an argument. See: > > 4eee492 drm/i915: fix driver's versions of WARN_ON & WARN_ON_ONCE > Great catch, I'll change it to have preceding "%s". Did not get any compile-time warning though, this would be something caught by a (rather advanced?) static analysis. Regards, Joonas > .Dave.
On Fri, Dec 18, 2015 at 02:11:24PM +0200, Joonas Lahtinen wrote: > On pe, 2015-12-18 at 12:03 +0000, Dave Gordon wrote: > > On 18/12/15 10:39, Joonas Lahtinen wrote: > > > Using __stringify(x) instead of #x adds support for macros as > > > a parameter and reduces runtime overhead. > > > > > > Slightly increases the .text size but should not matter. > > > > > > v2: > > > - Define I915_STATE_WARN_ON though I915_STATE_WARN > > > (Bikeshed inspiration by Chris) > > > > > > Cc: Rob Clark <robdclark@gmail.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 14 ++++---------- > > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > > > > > <SNIP> > > > > > > > \ > > > -}) > > > +#define I915_STATE_WARN_ON(x) > > > \ > > > + I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")") > > > > > > static inline const char *yesno(bool v) > > > { > > > > NAK. > > > > This will give compile-time warnings for lines such as: > > > > WARN_ON(x%16 != 0); > > > > because the stringified text of the expression (which in this case > > contains a "%" character) would appear as part of the format string, > > rather than inside an argument. See: > > > > 4eee492 drm/i915: fix driver's versions of WARN_ON & WARN_ON_ONCE > > > > Great catch, I'll change it to have preceding "%s". Did not get any > compile-time warning though, this would be something caught by a > (rather advanced?) static analysis. New gcc seems to complain about this. At least since a few weeks I've seen a few patches for kernel-wide changes to address this float about. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5a5a3e0..1ccd137 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -69,11 +69,11 @@ BUILD_BUG_ON(__i915_warn_cond); \ WARN(__i915_warn_cond, "WARN_ON(" #x ")"); }) #else -#define WARN_ON(x) WARN((x), "WARN_ON(%s)", #x ) +#define WARN_ON(x) WARN((x), "WARN_ON(" __stringify(x) ")") #endif #undef WARN_ON_ONCE -#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(%s)", #x ) +#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(" __stringify(x) ")") #define MISSING_CASE(x) WARN(1, "Missing switch case (%lu) in %s\n", \ (long) (x), __func__); @@ -93,14 +93,8 @@ unlikely(__ret_warn_on); \ }) -#define I915_STATE_WARN_ON(condition) ({ \ - int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on)) \ - if (!WARN(i915.verbose_state_checks, \ - "WARN_ON(" #condition ")\n")) \ - DRM_ERROR("WARN_ON(" #condition ")\n"); \ - unlikely(__ret_warn_on); \ -}) +#define I915_STATE_WARN_ON(x) \ + I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")") static inline const char *yesno(bool v) {