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.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, UNPARSEABLE_RELAY 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 B1207C433FE for ; Wed, 22 Sep 2021 04:24:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8DA0D60EE7 for ; Wed, 22 Sep 2021 04:24:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231425AbhIVEZo (ORCPT ); Wed, 22 Sep 2021 00:25:44 -0400 Received: from smtprelay0151.hostedemail.com ([216.40.44.151]:48684 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S229495AbhIVEZl (ORCPT ); Wed, 22 Sep 2021 00:25:41 -0400 Received: from omf09.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay04.hostedemail.com (Postfix) with ESMTP id D21BE180A5AF1; Wed, 22 Sep 2021 04:24:10 +0000 (UTC) Received: from [HIDDEN] (Authenticated sender: joe@perches.com) by omf09.hostedemail.com (Postfix) with ESMTPA id 33A581E04D9; Wed, 22 Sep 2021 04:24:06 +0000 (UTC) Message-ID: <0b1c78b395a7a198a089ba8f6283d8d10829720c.camel@perches.com> Subject: Re: function prototype element ordering From: Joe Perches To: Kees Cook , Linus Torvalds Cc: linux-kernel@vger.kernel.org, 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 Date: Tue, 21 Sep 2021 21:24:04 -0700 In-Reply-To: <202109211757.F38DF644@keescook> References: <20210909200948.090d4e213ca34b5ad1325a7e@linux-foundation.org> <20210910031046.G76dQvPhV%akpm@linux-foundation.org> <202109211630.2D00627@keescook> <202109211757.F38DF644@keescook> Content-Type: text/plain; charset="ISO-8859-1" User-Agent: Evolution 3.40.0-1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Stat-Signature: 66kfrtcpa4oq76ewff9t1btep61h55mx X-Rspamd-Server: rspamout05 X-Rspamd-Queue-Id: 33A581E04D9 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Session-ID: U2FsdGVkX1/uSb4yH+DYL9KIG9muU2DF8mmCwJIvhk8= X-HE-Tag: 1632284646-179228 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2021-09-21 at 19:25 -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: > > > > > > > > > > +__alloc_size(1) > > > > >  extern void *vmalloc(unsigned long size); > > > > [...] > > > > > > > > All of these are added in the wrong place - inconsistent with the very > > > > compiler documentation the patches add. > > > > > > > > The function attributes are generally added _after_ the function, > > > > although admittedly we've been quite confused here before. > > > > > > > > But the very compiler documentation you point to in the patch that > > > > adds these macros gives that as the examples both for gcc and clang: > > > > > > > > + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute > > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size > > > > > > > > and honestly I think that is the preferred format because this is > > > > about the *function*, not about the return type. > > > > > > > > Do both placements work? Yes. > > > > > > I'm cleaning this up now, and have discovered that the reason for the > > > before-function placement is consistency with static inlines. If I do this: > > > > > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __alloc_size(1) > > > { > > > ... > > > } > > > > > > GCC is very angry: > > > > > > ./include/linux/slab.h:519:1: error: attributes should be specified before the declarator in a function definition > > >   519 | static __always_inline void *kmalloc_large(size_t size, gfp_t flags) __alloc_size(1) > > >       | ^~~~~~ > > > > > > It's happy if I treat it as a "return type attribute" in the ordering, > > > though: > > > > > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, gfp_t flags) > > > > > > I'll do that unless you have a preference for somewhere else... > > > > _please_ put it before the return type on a separate line. > > > > [__attributes] > > [static inline const] function() > > Somehow Linus wasn't in CC. :P > > 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 ... > > Looking through what was written before[1] and through examples in the > source tree, I find the following categories: > > 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) > > Everyone seems to basically agree on: > > [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...) > > There is a lot of disagreement over where 5 and 6 should fit in above. And > there is a lot of confusion over 4 (mixed between before and after the > function name) and 2 (see below). > > 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__). > > 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: > > $ git grep 'static __init int' | wc -l > 349 > $ git grep 'static int __init' | wc -l > 8402 > > But it's clearly positioned like a return type attribute in most of the > tree. What's correct? Neither really. 'Correct' is such a difficult concept. 'Preferred' might be better. btw: there are about another 100 other uses with '__init' as the initial attribute, mostly in trace. And I still think that return type attributes like __init, which is just a __section define, should go before the function storage class and ideally on a separate line to simplify the parsing of the actual function declaration. Attributes like __section, __aligned, __cold, etc... don't have much value when looking up a function definition. > 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): Pathological is pretty common these days as the function name length is rather longer now than earlier times. > [storage class] [storage class attributes] [return type] [return type attributes] [function argument attributes] [name]([arg1type] [arg1name], ...) [function attributes] > > Joe appears to want (on two lines): > > [storage class attributes] [function attributes] [function argument attributes] > [storage class] [return type] [return type attributes] [name]([arg1type] [arg1name], ...) I would put [return type attributes] on the initial separate line even though that's not the most common use today. > I would just like to have an arrangement that won't get NAKed by > someone. ;) Bikeshed building dreamer... btw: Scouting through kernel code for frequency of use examples really should have some age of code checking associated to the use. Older code was far more freeform than more recently written code. But IMO the desire here is to ask for a bit more uniformity, not require it. 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.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, UNPARSEABLE_RELAY 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 3962AC433F5 for ; Wed, 22 Sep 2021 04:24:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B06E260EE7 for ; Wed, 22 Sep 2021 04:24:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B06E260EE7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=perches.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 1F6E16B006C; Wed, 22 Sep 2021 00:24:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 17E89900002; Wed, 22 Sep 2021 00:24:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 01D346B0073; Wed, 22 Sep 2021 00:24:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0154.hostedemail.com [216.40.44.154]) by kanga.kvack.org (Postfix) with ESMTP id DFBD76B006C for ; Wed, 22 Sep 2021 00:24:11 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 932C28249980 for ; Wed, 22 Sep 2021 04:24:11 +0000 (UTC) X-FDA: 78613917102.07.E3ED02C Received: from smtprelay.hostedemail.com (smtprelay0106.hostedemail.com [216.40.44.106]) by imf29.hostedemail.com (Postfix) with ESMTP id 4BB869000257 for ; Wed, 22 Sep 2021 04:24:11 +0000 (UTC) Received: from omf09.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay04.hostedemail.com (Postfix) with ESMTP id D21BE180A5AF1; Wed, 22 Sep 2021 04:24:10 +0000 (UTC) Received: from [HIDDEN] (Authenticated sender: joe@perches.com) by omf09.hostedemail.com (Postfix) with ESMTPA id 33A581E04D9; Wed, 22 Sep 2021 04:24:06 +0000 (UTC) Message-ID: <0b1c78b395a7a198a089ba8f6283d8d10829720c.camel@perches.com> Subject: Re: function prototype element ordering From: Joe Perches To: Kees Cook , Linus Torvalds Cc: linux-kernel@vger.kernel.org, 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 Date: Tue, 21 Sep 2021 21:24:04 -0700 In-Reply-To: <202109211757.F38DF644@keescook> References: <20210909200948.090d4e213ca34b5ad1325a7e@linux-foundation.org> <20210910031046.G76dQvPhV%akpm@linux-foundation.org> <202109211630.2D00627@keescook> <202109211757.F38DF644@keescook> Content-Type: text/plain; charset="ISO-8859-1" User-Agent: Evolution 3.40.0-1 MIME-Version: 1.0 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Session-ID: U2FsdGVkX1/uSb4yH+DYL9KIG9muU2DF8mmCwJIvhk8= X-HE-Tag: 1632284646-179228 X-Stat-Signature: zsrum97rjksed6dzctk4ud6c3my866yb Authentication-Results: imf29.hostedemail.com; dkim=none; dmarc=none; spf=none (imf29.hostedemail.com: domain of joe@perches.com has no SPF policy when checking 216.40.44.106) smtp.mailfrom=joe@perches.com X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 4BB869000257 X-HE-Tag: 1632284651-407070 Content-Transfer-Encoding: quoted-printable 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 Tue, 2021-09-21 at 19:25 -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) > > > > > =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 tha= t > > > > adds these macros gives that as the examples both for gcc and cla= ng: > > > >=20 > > > > + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Att= ributes.html#index-alloc_005fsize-function-attribute > > > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#al= loc-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 t= he > > > before-function placement is consistency with static inlines. If I = do this: > > >=20 > > > static __always_inline void * kmalloc(size_t size, gfp_t flags) __a= lloc_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 > > > =A0=A0519 | static __always_inline void *kmalloc_large(size_t size,= gfp_t flags) __alloc_size(1) > > > =A0=A0=A0=A0=A0=A0| ^~~~~~ > > >=20 > > > It's happy if I treat it as a "return type attribute" in the orderi= ng, > > > though: > > >=20 > > > static __always_inline void * __alloc_size(1) kmalloc(size_t size, = gfp_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. = And > 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? Neither really. 'Correct' is such a difficult concept. 'Preferred' might be better. btw: there are about another 100 other uses with '__init' as the initial attribute, mostly in trace. And I still think that return type attributes like __init, which is just a __section define, should go before the function storage class and ideally on a separate line to simplify the parsing of the actual function declaration. Attributes like __section, __aligned, __cold, etc... don't have much value when looking up a function definition. > Regardless, given the constraints above, it seems like what Linus may > want is (on "one line", though it will get wrapped in pathological case= s > like kmem_cache_alloc_node_trace): Pathological is pretty common these days as the function name length is rather longer now than earlier times. =20 > [storage class] [storage class attributes] [return type] [return type a= ttributes] [function argument attributes] [name]([arg1type] [arg1name], .= ..) [function attributes] >=20 > Joe appears to want (on two lines): >=20 > [storage class attributes] [function attributes] [function argument att= ributes] > [storage class] [return type] [return type attributes] [name]([arg1type= ] [arg1name], ...) I would put [return type attributes] on the initial separate line even though that's not the most common use today. > I would just like to have an arrangement that won't get NAKed by > someone. ;) Bikeshed building dreamer... btw: Scouting through kernel code for frequency of use examples really should have some age of code checking associated to the use. Older code was far more freeform than more recently written code. But IMO the desire here is to ask for a bit more uniformity, not require it.