From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1AAC1C433FE for ; Wed, 22 Sep 2021 11:19:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B82186127B for ; Wed, 22 Sep 2021 11:19:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B82186127B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 1EBF56B006C; Wed, 22 Sep 2021 07:19:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 19B986B0072; Wed, 22 Sep 2021 07:19:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 06388900002; Wed, 22 Sep 2021 07:19:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0056.hostedemail.com [216.40.44.56]) by kanga.kvack.org (Postfix) with ESMTP id EBC7B6B006C for ; Wed, 22 Sep 2021 07:19:49 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 9052D2DD6C for ; Wed, 22 Sep 2021 11:19:49 +0000 (UTC) X-FDA: 78614964498.30.11149E4 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by imf30.hostedemail.com (Postfix) with ESMTP id 7BBF6E001990 for ; Wed, 22 Sep 2021 11:19:48 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10114"; a="203065013" X-IronPort-AV: E=Sophos;i="5.85,313,1624345200"; d="scan'208";a="203065013" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Sep 2021 04:19:46 -0700 X-IronPort-AV: E=Sophos;i="5.85,313,1624345200"; d="scan'208";a="557422965" Received: from vidyaram-mobl1.gar.corp.intel.com (HELO localhost) ([10.251.218.73]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Sep 2021 04:19:39 -0700 From: Jani Nikula To: Alexey Dobriyan , linux-kernel@vger.kernel.org Cc: Linus Torvalds , Joe Perches , Andrew Morton , apw@canonical.com, Christoph Lameter , Daniel Micay , Dennis Zhou , dwaipayanray1@gmail.com, Joonsoo Kim , Linux-MM , Lukas Bulwahn , mm-commits@vger.kernel.org, Nathan Chancellor , Nick Desaulniers , Miguel Ojeda , Pekka Enberg , David Rientjes , Tejun Heo , Vlastimil Babka , linux-doc@vger.kernel.org Subject: Re: function prototype element ordering In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20210909200948.090d4e213ca34b5ad1325a7e@linux-foundation.org> <20210910031046.G76dQvPhV%akpm@linux-foundation.org> <202109211630.2D00627@keescook> <202109211757.F38DF644@keescook> Date: Wed, 22 Sep 2021 14:19:28 +0300 Message-ID: <874kacn9hb.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Authentication-Results: imf30.hostedemail.com; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=intel.com (policy=none); spf=none (imf30.hostedemail.com: domain of jani.nikula@intel.com has no SPF policy when checking 192.55.52.136) smtp.mailfrom=jani.nikula@intel.com X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 7BBF6E001990 X-Stat-Signature: 85ydttxx3thzywacceo8gz58pdobgkci X-HE-Tag: 1632309588-363407 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, 22 Sep 2021, Alexey Dobriyan wrote: > On Tue, Sep 21, 2021 at 07:25:53PM -0700, Kees Cook wrote: >> On Tue, Sep 21, 2021 at 04:45:44PM -0700, Joe Perches wrote: >> > On Tue, 2021-09-21 at 16:37 -0700, Kees Cook wrote: >> > > On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote: >> > > > On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton wrote: >> > > > >=20 >> > > > > +__alloc_size(1) >> > > > > =C2=A0extern void *vmalloc(unsigned long size); >> > > > [...] >> > > >=20 >> > > > All of these are added in the wrong place - inconsistent with the = very >> > > > compiler documentation the patches add. >> > > >=20 >> > > > The function attributes are generally added _after_ the function, >> > > > although admittedly we've been quite confused here before. >> > > >=20 >> > > > But the very compiler documentation you point to in the patch that >> > > > adds these macros gives that as the examples both for gcc and clan= g: >> > > >=20 >> > > > + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attr= ibutes.html#index-alloc_005fsize-function-attribute >> > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#all= oc-size >> > > >=20 >> > > > and honestly I think that is the preferred format because this is >> > > > about the *function*, not about the return type. >> > > >=20 >> > > > Do both placements work? Yes. >> > >=20 >> > > I'm cleaning this up now, and have discovered that the reason for the >> > > before-function placement is consistency with static inlines. If I d= o this: >> > >=20 >> > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __al= loc_size(1) >> > > { >> > > ... >> > > } >> > >=20 >> > > GCC is very angry: >> > >=20 >> > > ./include/linux/slab.h:519:1: error: attributes should be specified = before the declarator in a function definition >> > > =C2=A0=C2=A0519 | static __always_inline void *kmalloc_large(size_t = size, gfp_t flags) __alloc_size(1) >> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0| ^~~~~~ >> > >=20 >> > > It's happy if I treat it as a "return type attribute" in the orderin= g, >> > > though: >> > >=20 >> > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, g= fp_t flags) >> > >=20 >> > > I'll do that unless you have a preference for somewhere else... >> >=20 >> > _please_ put it before the return type on a separate line. >> >=20 >> > [__attributes] >> > [static inline const] function() >>=20 >> Somehow Linus wasn't in CC. :P >>=20 >> Linus, what do you want here? I keep getting conflicting (or >> uncompilable) advice. I'm also trying to prepare a patch for >> Documentation/process/coding-style.rst ... >>=20 >> Looking through what was written before[1] and through examples in the >> source tree, I find the following categories: >>=20 >> 1- storage class: static extern inline __always_inline >> 2- storage class attributes/hints/???: __init __cold >> 3- return type: void * >> 4- return type attributes: __must_check __noreturn __assume_aligned(n) >> 5- function attributes: __attribute_const__ __malloc >> 6- function argument attributes: __printf(n, m) __alloc_size(n) >>=20 >> Everyone seems to basically agree on: >>=20 >> [storage class] [return type] [return type attributes] [name]([arg1type]= [arg1name], ...) >>=20 >> There is a lot of disagreement over where 5 and 6 should fit in above. A= nd >> there is a lot of confusion over 4 (mixed between before and after the >> function name) and 2 (see below). >>=20 >> What's currently blocking me is that 6 cannot go after the function >> (for definitions) because it angers GCC (see quoted bit above), but 5 >> can (e.g. __attribute_const__). >>=20 >> Another inconsistency seems to be 2 (mainly section markings like >> __init). Sometimes it's after the storage class and sometimes after the >> return type, but it certainly feels more like a storage class than a >> return type attribute: >>=20 >> $ git grep 'static __init int' | wc -l >> 349 >> $ git grep 'static int __init' | wc -l >> 8402 >>=20 >> But it's clearly positioned like a return type attribute in most of the >> tree. What's correct? >>=20 >> Regardless, given the constraints above, it seems like what Linus may >> want is (on "one line", though it will get wrapped in pathological cases >> like kmem_cache_alloc_node_trace): >>=20 >> [storage class] [storage class attributes] [return type] [return type at= tributes] [function argument attributes] [name]([arg1type] [arg1name], ...)= [function attributes] >>=20 >> Joe appears to want (on two lines): >>=20 >> [storage class attributes] [function attributes] [function argument attr= ibutes] >> [storage class] [return type] [return type attributes] [name]([arg1type]= [arg1name], ...) >>=20 >> I would just like to have an arrangement that won't get NAKed by >> someone. ;) And I'm willing to document it. :) > > Attributes should be on their own line, they can be quite lengthy. > > __attribute__((...)) > [static] [inline] T f(A1 arg1, ...) > { > ... > } > > There will be even more attributes in the future, both added by > compilers and developers (const, pure, WUR), so let's make "prototype lan= e" > for them. > > Same for structures: > > __attribute__((packed)) > struct S { > }; > > Kernel practice of hiding attributes under defines (__ro_after_init) > breaks ctags which parses the last identifier before semicolon as object > name. Naturally, it is ctags bug, but placing attributes before > declaration will autmatically unbreak such cases. git grep seems to suggest __packed is preferred over __attribute__((packed)), and at the end of the struct declaration instead of at front: struct S { /* ... */ } __packed; And GNU Global handles this just fine. ;) BR, Jani. --=20 Jani Nikula, Intel Open Source Graphics Center