Re: [PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings

From: Dave Gordon <david.s.gordon@intel.com>
To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Intel graphics driver community testing & development
	<intel-gfx@lists.freedesktop.org>,
	Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings
Date: Fri, 18 Dec 2015 12:03:11 +0000	[thread overview]
Message-ID: <5673F5FF.6060302@intel.com> (raw)
In-Reply-To: <1450435157-17318-3-git-send-email-joonas.lahtinen@linux.intel.com>

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx