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

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 <redacted>
---
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

On Thu, Sep 30, 2021 at 4:58 PM Kees Cook [off-list ref] 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 <redacted>

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.

-- 
Thanks,
~Nick Desaulniers

On Fri, 2021-10-01 at 12:05 -0700, Nick Desaulniers wrote:

On Thu, Sep 30, 2021 at 4:58 PM Kees Cook [off-list ref] wrote:

quoted

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 <redacted>

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.

On Fri, Oct 01, 2021 at 12:05:25PM -0700, Nick Desaulniers wrote:

On Thu, Sep 30, 2021 at 4:58 PM Kees Cook [off-list ref] wrote:

quoted

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 <redacted>

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

On Fri, Oct 01, 2021 at 12:05:25PM -0700, Nick Desaulniers wrote:

On Thu, Sep 30, 2021 at 4:58 PM Kees Cook [off-list ref] wrote:

quoted

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 <redacted>

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

On Sat, Oct 2, 2021 at 8:31 AM Greg KH [off-list ref] 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

On Fri, Oct 1, 2021 at 1:59 AM Kees Cook [off-list ref] 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

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.

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.

On Sat, Oct 2, 2021 at 5:22 PM Joe Perches [off-list ref] 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

On Sat, Oct 2, 2021 at 5:22 PM Joe Perches [off-list ref] 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

On Sat, Oct 02, 2021 at 06:27:02PM +0200, Miguel Ojeda wrote:

On Sat, Oct 2, 2021 at 5:22 PM Joe Perches [off-list ref] wrote:

quoted

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

Kees Cook [off-list ref] 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 <redacted>
---
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

On Tue, Oct 5, 2021 at 1:57 AM Jonathan Corbet [off-list ref] 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

On Tue, Oct 05, 2021 at 10:28:40AM +0200, Miguel Ojeda wrote:

On Tue, Oct 5, 2021 at 1:57 AM Jonathan Corbet [off-list ref] wrote:

quoted

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