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 <[email protected]>
Reported-by: Dmitry Vyukov <[email protected]>
Cc: Joe Perches <[email protected]>
---
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
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.
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 <[email protected]>
> Reported-by: Dmitry Vyukov <[email protected]>
> Cc: Joe Perches <[email protected]>
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;
>
Jason Baron <[email protected]> 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 <[email protected]>
>> Reported-by: Dmitry Vyukov <[email protected]>
>> Cc: Joe Perches <[email protected]>
>
> 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.
On Fri, 2015-12-04 at 11:38 -0500, Aaron Conole wrote:
> Jason Baron <[email protected]> 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 <[email protected]>
> > > Reported-by: Dmitry Vyukov <[email protected]>
> > > Cc: Joe Perches <[email protected]>
> >
> > 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.
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 <[email protected]>
Reported-by: Dmitry Vyukov <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jason Baron <[email protected]>
---
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
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 <[email protected]>
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)
Arnd Bergmann <[email protected]> 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 <[email protected]>
>
> 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.
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.