Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755220AbbLDQjB (ORCPT ); Fri, 4 Dec 2015 11:39:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45988 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754001AbbLDQjA (ORCPT ); Fri, 4 Dec 2015 11:39:00 -0500 From: Aaron Conole To: Jason Baron Cc: Andrew Morton , linux-kernel@vger.kernel.org, Joe Perches Subject: Re: [PATCH] printk: fix pr_debug and pr_devel to elide function calls References: <1449182754-19088-1-git-send-email-aconole@redhat.com> <5661BFEE.1050103@akamai.com> Date: Fri, 04 Dec 2015 11:38:59 -0500 In-Reply-To: <5661BFEE.1050103@akamai.com> (Jason Baron's message of "Fri, 4 Dec 2015 11:31:42 -0500") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2374 Lines: 62 Jason Baron 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 >> 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 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. -- 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/