Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755338AbbLDQbp (ORCPT ); Fri, 4 Dec 2015 11:31:45 -0500 Received: from prod-mail-xrelay07.akamai.com ([23.79.238.175]:13493 "EHLO prod-mail-xrelay07.akamai.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754960AbbLDQbo (ORCPT ); Fri, 4 Dec 2015 11:31:44 -0500 Subject: Re: [PATCH] printk: fix pr_debug and pr_devel to elide function calls To: Aaron Conole , Andrew Morton References: <1449182754-19088-1-git-send-email-aconole@redhat.com> Cc: linux-kernel@vger.kernel.org, Joe Perches From: Jason Baron X-Enigmail-Draft-Status: N1110 Message-ID: <5661BFEE.1050103@akamai.com> Date: Fri, 4 Dec 2015 11:31:42 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1449182754-19088-1-git-send-email-aconole@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4964 Lines: 150 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 > Reported-by: Dmitry Vyukov > Cc: Joe Perches 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 > @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/