Re: [PATCH] printk: fix pr_debug and pr_devel to elide function calls

[Jason Baron]

On 12/03/2015 05:45 PM, Aaron Conole wrote:
> Currently, pr_debug and pr_devel will not elide function call arguments
> appearing in calls to no_printk for these macros. This is because all
> side effects must be honored before proceeding to the 0-value assignment
> in no_printk.
> 
> The behavior is contrary to documentation found in the CodingStyle and
> header file where these functions are declared. 
> 
> This patch corrects that behavior by shunting out the call to no_printk
> completely. The format string is still checked by gcc for correctness, but
> no code seems to be emitted in common cases.
> 
> fixes commit 5264f2f75d86 ("include/linux/printk.h: use and neaten
> no_printk")
> 
> Signed-off-by: Aaron Conole <aconole@xxxxxxxxxx>
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Joe Perches <joe@xxxxxxxxxxx>

I think we should just convert no_printk() to not emit anything. This
will avoid us adding unwrapped calls to 'no_printk()' in the future, and
I think makes the code more readable. Based on Joe's previous
'eliminated_printk()' thing. IE:

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..58632bf 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -108,11 +108,11 @@ struct va_format {
  * Dummy printk for disabled debugging statements to use whilst maintaining
  * gcc's format and side-effect checking.
  */
-static inline __printf(1, 2)
-int no_printk(const char *fmt, ...)
-{
-       return 0;
-}
+#define no_printk(fmt, ...)                    \
+do {                                           \
+       if (0)                                  \
+               printk(fmt, ##__VA_ARGS__);     \
+} while (0)

 #ifdef CONFIG_EARLY_PRINTK
 extern asmlinkage __printf(1, 2)

Thanks,

-Jason

> 
> ---
>  include/linux/printk.h | 42 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..87dfdd2 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -270,8 +270,12 @@ extern asmlinkage void dump_stack(void) __cold;
>  #define pr_devel(fmt, ...) \
>  	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>  #else
> -#define pr_devel(fmt, ...) \
> -	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_devel(fmt, ...)					\
> +do {								\
> +	if (0) {						\
> +		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);\
> +	}							 \
> +} while (0)
>  #endif
>  
>  #include <linux/dynamic_debug.h>
> @@ -286,7 +290,11 @@ extern asmlinkage void dump_stack(void) __cold;
>  	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>  #else
>  #define pr_debug(fmt, ...) \
> -	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +do {								\
> +	if (0) {						\
> +		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);\
> +	}							 \
> +} while (0)
>  #endif
>  
>  /*
> @@ -341,7 +349,11 @@ extern asmlinkage void dump_stack(void) __cold;
>  	printk_once(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>  #else
>  #define pr_devel_once(fmt, ...)					\
> -	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +do {								\
> +	if (0) {						\
> +		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);\
> +	}							 \
> +} while (0)
>  #endif
>  
>  /* If you are writing a driver, please use dev_dbg instead */
> @@ -350,7 +362,11 @@ extern asmlinkage void dump_stack(void) __cold;
>  	printk_once(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>  #else
>  #define pr_debug_once(fmt, ...)					\
> -	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +do {								\
> +	if (0) {						\
> +		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);\
> +	}							 \
> +} while (0)
>  #endif
>  
>  /*
> @@ -392,8 +408,12 @@ extern asmlinkage void dump_stack(void) __cold;
>  #define pr_devel_ratelimited(fmt, ...)					\
>  	printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>  #else
> -#define pr_devel_ratelimited(fmt, ...)					\
> -	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_devel_ratelimited(fmt, ...)				\
> +do {								\
> +	if (0) {						\
> +		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);\
> +	}							 \
> +} while (0)
>  #endif
>  
>  /* If you are writing a driver, please use dev_dbg instead */
> @@ -413,8 +433,12 @@ do {									\
>  #define pr_debug_ratelimited(fmt, ...)					\
>  	printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>  #else
> -#define pr_debug_ratelimited(fmt, ...) \
> -	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_debug_ratelimited(fmt, ...)					\
> +do {								\
> +	if (0) {						\
> +		no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__);\
> +	}							 \
> +} while (0)
>  #endif
>  
>  extern const struct file_operations kmsg_fops;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/