[PATCH v4] docs: Explain the desired position of function attributes

[PATCH v4] docs: Explain the desired position of function attributes

[Kees Cook]

While discussing how to format the addition of various function
attributes, some "unwritten rules" of ordering surfaced[1]. Capture as
close as possible to Linus's preferences for future reference.

(Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and
others that would prefer all attributes live on a separate leading line.)

[1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v4:
- fix another stray "void"! This is why code needs a compiler... (thx randy)
---
 Documentation/process/coding-style.rst | 30 ++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 42969ab37b34..45b48510f5ec 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -487,6 +487,36 @@ because it is a simple way to add valuable information for the reader.
 Do not use the ``extern`` keyword with function prototypes as this makes
 lines longer and isn't strictly necessary.
 
+When writing a function declarations, please keep the `order of elements regular
+<https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/>`_.
+For example::
+
+ extern __init void * __must_check action(enum magic value, size_t size, u8 count,
+ 					  char *fmt, ...) __printf(4, 5) __malloc;
+
+The preferred order of elements for a function prototype is:
+
+- storage class (here, ``extern``, and things like ``static __always_inline`` even though
+  ``__always_inline`` is technically an attribute, it is treated like ``inline``)
+- storage class attributes (here, ``__init`` -- i.e. section declarations, but also things like ``__cold``)
+- return type (here, ``void *``)
+- return type attributes (here, ``__must_check``)
+- function name (here, ``action``)
+- function parameters (here, ``(enum magic value, size_t size, u8 count, char *fmt, ...)``, noting that parameter names should always be included)
+- function parameter attributes (here, ``__printf(4, 5)``)
+- function behavior attributes (here, ``__malloc``)
+
+Note that for a function definition (e.g. ``static inline``), the compiler does
+not allow function parameter attributes after the function parameters. In these
+cases, they should go after the storage class attributes (e.g. note the changed
+position of ``__printf(4, 5)``)::
+
+ static __always_inline __init __printf(4, 5) void * __must_check action(enum magic value,
+ 		size_t size, u8 count, char *fmt, ...)
+ 		__malloc
+ {
+ 	...
+ }
 
 7) Centralized exiting of functions
 -----------------------------------
-- 
2.30.2

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Nick Desaulniers]

On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> While discussing how to format the addition of various function
> attributes, some "unwritten rules" of ordering surfaced[1]. Capture as
> close as possible to Linus's preferences for future reference.
>
> (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and
> others that would prefer all attributes live on a separate leading line.)
>
> [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

While I appreciate you getting the ball across the finish line (having
_any_ documentation to point to in future bikesheds), I can't help but
shake the feeling that the chosen policy will harm the ability of
existing automated code formatting tools from being able to automate
code formatting on the kernel.

> ---
> v4:
> - fix another stray "void"! This is why code needs a compiler... (thx randy)
> ---
>  Documentation/process/coding-style.rst | 30 ++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 42969ab37b34..45b48510f5ec 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -487,6 +487,36 @@ because it is a simple way to add valuable information for the reader.
>  Do not use the ``extern`` keyword with function prototypes as this makes
>  lines longer and isn't strictly necessary.
>
> +When writing a function declarations, please keep the `order of elements regular
> +<https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/>`_.
> +For example::
> +
> + extern __init void * __must_check action(enum magic value, size_t size, u8 count,
> +                                         char *fmt, ...) __printf(4, 5) __malloc;
> +
> +The preferred order of elements for a function prototype is:
> +
> +- storage class (here, ``extern``, and things like ``static __always_inline`` even though
> +  ``__always_inline`` is technically an attribute, it is treated like ``inline``)
> +- storage class attributes (here, ``__init`` -- i.e. section declarations, but also things like ``__cold``)
> +- return type (here, ``void *``)
> +- return type attributes (here, ``__must_check``)
> +- function name (here, ``action``)
> +- function parameters (here, ``(enum magic value, size_t size, u8 count, char *fmt, ...)``, noting that parameter names should always be included)
> +- function parameter attributes (here, ``__printf(4, 5)``)
> +- function behavior attributes (here, ``__malloc``)
> +
> +Note that for a function definition (e.g. ``static inline``), the compiler does
> +not allow function parameter attributes after the function parameters. In these
> +cases, they should go after the storage class attributes (e.g. note the changed
> +position of ``__printf(4, 5)``)::
> +
> + static __always_inline __init __printf(4, 5) void * __must_check action(enum magic value,
> +               size_t size, u8 count, char *fmt, ...)
> +               __malloc
> + {
> +       ...
> + }
>
>  7) Centralized exiting of functions
>  -----------------------------------
> --
> 2.30.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Joe Perches]

On Fri, 2021-10-01 at 12:05 -0700, Nick Desaulniers wrote:
> On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote:
> > 
> > While discussing how to format the addition of various function
> > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as
> > close as possible to Linus's preferences for future reference.
> > 
> > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and
> > others that would prefer all attributes live on a separate leading line.)
> > 
> > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> While I appreciate you getting the ball across the finish line (having
> _any_ documentation to point to in future bikesheds), I can't help but
> shake the feeling that the chosen policy will harm the ability of
> existing automated code formatting tools from being able to automate
> code formatting on the kernel.

yup.

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Kees Cook]

On Fri, Oct 01, 2021 at 12:05:25PM -0700, Nick Desaulniers wrote:
> On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > While discussing how to format the addition of various function
> > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as
> > close as possible to Linus's preferences for future reference.
> >
> > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and
> > others that would prefer all attributes live on a separate leading line.)
> >
> > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> While I appreciate you getting the ball across the finish line (having
> _any_ documentation to point to in future bikesheds), I can't help but
> shake the feeling that the chosen policy will harm the ability of
> existing automated code formatting tools from being able to automate
> code formatting on the kernel.

I am but the messenger here. Is there something specific that'll break
if we follow this?

-- 
Kees Cook

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Greg KH]

On Fri, Oct 01, 2021 at 12:05:25PM -0700, Nick Desaulniers wrote:
> On Thu, Sep 30, 2021 at 4:58 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > While discussing how to format the addition of various function
> > attributes, some "unwritten rules" of ordering surfaced[1]. Capture as
> > close as possible to Linus's preferences for future reference.
> >
> > (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and
> > others that would prefer all attributes live on a separate leading line.)
> >
> > [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> While I appreciate you getting the ball across the finish line (having
> _any_ documentation to point to in future bikesheds), I can't help but
> shake the feeling that the chosen policy will harm the ability of
> existing automated code formatting tools from being able to automate
> code formatting on the kernel.

Why would documenting the expected format have an affect on tools not
being able to follow that exact expected format?  Are we defining a
format that is somehow impossible for them to use?

If anything I would think that now we have a format that the tools can
actually follow, while before it was semi-random as to what to pick as
the "one true way".

What am I missing here?

thanks,

greg k-h

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Miguel Ojeda]

On Sat, Oct 2, 2021 at 8:31 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Why would documenting the expected format have an affect on tools not
> being able to follow that exact expected format?  Are we defining a
> format that is somehow impossible for them to use?

From a quick test, clang-format-12 with our current config keeps
attributes in the same line and it does not seem to reorder them, so
it seems OK (the developer has to do it by hand, but that is fine)
except for the `__malloc` in the second example which is in a
different line (but not in the first). Is that intended?

> If anything I would think that now we have a format that the tools can
> actually follow, while before it was semi-random as to what to pick as
> the "one true way".

In the future, clang-format could have a configuration option to pass
a sort order, in which case, having the sort order already defined in
the kernel would definitely be helpful.

In fact, we could use the fact that the kernel has one as a way to
tell upstream that such a feature would be nice to have :)

Cheers,
Miguel

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Miguel Ojeda]

On Fri, Oct 1, 2021 at 1:59 AM Kees Cook <keescook@chromium.org> wrote:
>
> + static __always_inline __init __printf(4, 5) void * __must_check action(enum magic value,
> +               size_t size, u8 count, char *fmt, ...)
> +               __malloc

(From my other email) Is this `__malloc` intended to be in a separate
line? (the first example has everything on the same line).

Cheers,
Miguel

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Joe Perches]

On Sat, 2021-10-02 at 08:31 +0200, Greg KH wrote:
> I would think that now we have a format that the tools can
> actually follow, while before it was semi-random as to what to pick as
> the "one true way".

There will never be 'one true (and universal) way'.

Most existing code doesn't follow the suggested formatting and you
can't require or expect the existing code to change.

If automated scripts exist to change all the code to that new
'one true way', it wouldn't be applied.
 
> What am I missing here?

Try writing a regex for the proposal and make sure to separate out
all the various __<foo> attributes into their proper locations...

yuck and g'luck.

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Joe Perches]

On Sat, 2021-10-02 at 12:42 +0200, Miguel Ojeda wrote:
> In the future, clang-format could have a configuration option to pass
> a sort order, in which case, having the sort order already defined in
> the kernel would definitely be helpful.

It's not so much a sort order so much as it's a positional one.

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Miguel Ojeda]

On Sat, Oct 2, 2021 at 5:22 PM Joe Perches <joe@perches.com> wrote:
>
> If automated scripts exist to change all the code to that new
> 'one true way', it wouldn't be applied.

I think nobody is saying we should reformat all code at once, just
that agreeing on a given style allows us to eventually automate it (it
also makes humans more likely to be consistent).

> Try writing a regex for the proposal and make sure to separate out
> all the various __<foo> attributes into their proper locations...

It does not need to be a regex...

Cheers,
Miguel

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Miguel Ojeda]

On Sat, Oct 2, 2021 at 5:22 PM Joe Perches <joe@perches.com> wrote:
>
> It's not so much a sort order so much as it's a positional one.

Sure, there are two parts, the order w.r.t. the signature parts (e.g.
"before return type") and the order between the attributes themselves.
Both are included in what I meant.

By the way, clang-format-13 already has a way to pass it a list of the
"attribute macros" (such as `__unused`), so adding more information on
top seems reasonable.

Cheers,
Miguel

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Kees Cook]

On Sat, Oct 02, 2021 at 06:27:02PM +0200, Miguel Ojeda wrote:
> On Sat, Oct 2, 2021 at 5:22 PM Joe Perches <joe@perches.com> wrote:
> >
> > If automated scripts exist to change all the code to that new
> > 'one true way', it wouldn't be applied.
> 
> I think nobody is saying we should reformat all code at once, just
> that agreeing on a given style allows us to eventually automate it (it
> also makes humans more likely to be consistent).

I just want to know what style to use to not have it NAKed by Linus. ;)

But yeah, it might be nice to do style checking against the kernel some
day. We have a lot of weird stuff in here, though! *slaps roof of kernel*

-- 
Kees Cook

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Jonathan Corbet]

Kees Cook <keescook@chromium.org> writes:

> While discussing how to format the addition of various function
> attributes, some "unwritten rules" of ordering surfaced[1]. Capture as
> close as possible to Linus's preferences for future reference.
>
> (Though I note the dissent voiced by Joe Perches, Alexey Dobriyan, and
> others that would prefer all attributes live on a separate leading line.)
>
> [1] https://lore.kernel.org/mm-commits/CAHk-=wiOCLRny5aifWNhr621kYrJwhfURsa0vFPeUEm8mF0ufg@mail.gmail.com/
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v4:
> - fix another stray "void"! This is why code needs a compiler... (thx randy)
> ---
>  Documentation/process/coding-style.rst | 30 ++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Did I ever mention that I *hate* coding-style patches? :)

In this case I think we're as close as to consensus as things get.  In
the absence of a strong reason to the contrary, I'll apply this before
too long.

Thanks,

jon

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Miguel Ojeda]

On Tue, Oct 5, 2021 at 1:57 AM Jonathan Corbet <corbet@lwn.net> wrote:
>
> In this case I think we're as close as to consensus as things get.  In
> the absence of a strong reason to the contrary, I'll apply this before
> too long.

No strong reason, but there was the question about the `__malloc` in a
separate line in the second example which seems to contradict the
declaration and it is not explained otherwise (+ clang-format does it
differently).

Cheers,
Miguel

Re: [PATCH v4] docs: Explain the desired position of function attributes

[Kees Cook]

On Tue, Oct 05, 2021 at 10:28:40AM +0200, Miguel Ojeda wrote:
> On Tue, Oct 5, 2021 at 1:57 AM Jonathan Corbet <corbet@lwn.net> wrote:
> >
> > In this case I think we're as close as to consensus as things get.  In
> > the absence of a strong reason to the contrary, I'll apply this before
> > too long.
> 
> No strong reason, but there was the question about the `__malloc` in a
> separate line in the second example which seems to contradict the
> declaration and it is not explained otherwise (+ clang-format does it
> differently).

I'll send a v5 -- the "extern" also needs to be dropped.

-- 
Kees Cook