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

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

[Aaron Conole]

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@redhat.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Joe Perches <joe@perches.com>

---
 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;
-- 
2.6.1.133.gf5b6079

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

[Joe Perches]

On Thu, 2015-12-03 at 17:45 -0500, 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")
> 

The same should/could probably be done for dev_dbg/dev_vdbg
and the netdev and netif variants.

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@redhat.com>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Joe Perches <joe@perches.com>

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;
> 

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

[Aaron Conole]

Jason Baron <jbaron@akamai.com> writes:

> 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@redhat.com>
>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>> Cc: Joe Perches <joe@perches.com>
>
> 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

I like this fix the best, but reading some other upstream mails it seems
like that approach isn't likely to be accepted? I'll happily respin to
have your proposed code because it makes the most sense, if no one else
has any objections.

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

[Joe Perches]

On Fri, 2015-12-04 at 11:38 -0500, Aaron Conole wrote:
> Jason Baron <jbaron@akamai.com> writes:
> > 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@redhat.com>
> > > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: Joe Perches <joe@perches.com>
> > 
> > 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
> 
> I like this fix the best, but reading some other upstream mails it seems
> like that approach isn't likely to be accepted? I'll happily respin to
> have your proposed code because it makes the most sense, if no one else
> has any objections.

I have no objections.  1995 was a good year.

[PATCH v2] printk: help pr_debug and pr_devel to optimize out arguments

[Aaron Conole]

Currently, pr_debug and pr_devel will not elide function call arguments
appearing in calls to the 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
the 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@redhat.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Joe Perches <joe@perches.com>
Cc: Jason Baron <jbaron@akamai.com>
---
v1->v2 - Change no_printk behavior to elide arguments instead of modifying
individual pr_* macros, per Jason Baron's suggestion.

 include/linux/printk.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9729565..cc3803a 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -106,13 +106,14 @@ struct va_format {
 
 /*
  * Dummy printk for disabled debugging statements to use whilst maintaining
- * gcc's format and side-effect checking.
+ * gcc's format 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)
-- 
2.6.1.133.gf5b6079

Re: [PATCH v2] printk: help pr_debug and pr_devel to optimize out arguments

[Arnd Bergmann]

On Friday 04 December 2015 16:51:42 Aaron Conole wrote:
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -106,13 +106,14 @@ struct va_format {
>  
>  /*
>   * Dummy printk for disabled debugging statements to use whilst maintaining
> - * gcc's format and side-effect checking.
> + * gcc's format 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)
>  

This change breaks compiling lib/842/, at least in some configurations:

lib/842/842_decompress.c: In function '__do_index':
lib/842/842_decompress.c:205:12422: error: implicit declaration of function 'no_printk'

Using a gcc style vararg macro instead of the C99 style makes it work, but
I don't know why the original version didn't work.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/include/linux/printk.h b/include/linux/printk.h
index cc3803a8f73e..4d1851a82254 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -108,10 +108,10 @@ struct va_format {
  * Dummy printk for disabled debugging statements to use whilst maintaining
  * gcc's format checking.
  */
-#define no_printk(fmt, ...)			\
+#define no_printk(arg ...)			\
 do {						\
 	if (0) {				\
-		printk(fmt, ##__VA_ARGS__);	\
+		printk(arg);			\
 	}					\
 } while (0)
 

Re: [PATCH v2] printk: help pr_debug and pr_devel to optimize out arguments

[Aaron Conole]

Arnd Bergmann <arnd@arndb.de> writes:
> On Friday 04 December 2015 16:51:42 Aaron Conole wrote:
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -106,13 +106,14 @@ struct va_format {
>>  
>>  /*
>>   * Dummy printk for disabled debugging statements to use whilst maintaining
>> - * gcc's format and side-effect checking.
>> + * gcc's format 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)
>>  
>
> This change breaks compiling lib/842/, at least in some configurations:
>
> lib/842/842_decompress.c: In function '__do_index':
> lib/842/842_decompress.c:205:12422: error: implicit declaration of function 'no_printk'
>
> Using a gcc style vararg macro instead of the C99 style makes it work, but
> I don't know why the original version didn't work.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index cc3803a8f73e..4d1851a82254 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -108,10 +108,10 @@ struct va_format {
>   * Dummy printk for disabled debugging statements to use whilst maintaining
>   * gcc's format checking.
>   */
> -#define no_printk(fmt, ...)			\
> +#define no_printk(arg ...)			\
>  do {						\
>  	if (0) {				\
> -		printk(fmt, ##__VA_ARGS__);	\
> +		printk(arg);			\
>  	}					\
>  } while (0)
>  

Ugh. Sorry about this; I thought I had compiled everywhere but I guess not.

Re: [PATCH v2] printk: help pr_debug and pr_devel to optimize out arguments

[Joe Perches]

On Wed, 2015-12-09 at 12:52 +0100, Arnd Bergmann wrote:
> On Friday 04 December 2015 16:51:42 Aaron Conole wrote:
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -106,13 +106,14 @@ struct va_format {
> >  
> >  /*
> >   * Dummy printk for disabled debugging statements to use whilst maintaining
> > - * gcc's format and side-effect checking.
> > + * gcc's format 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)
> >  
> 
> This change breaks compiling lib/842/, at least in some configurations:
> 
> lib/842/842_decompress.c: In function '__do_index':
> lib/842/842_decompress.c:205:12422: error: implicit declaration of function 'no_printk'

Please specify a configuration that fails.