[RFC] bpf.2: Use standard types and attributes

  Some manual pages are already using C99 syntax for integral
types 'uint32_t', but some aren't.  There are some using kernel
syntax '__u32'.  Fix those.

Some pages also document attributes, using GNU syntax
'__attribute__((xxx))'.  Update those to use the shorter and more
portable C2x syntax, which hasn't been standardized yet, but is
already implemented in GCC, and available through either --std=c2x
or any of the --std=gnu... options.

Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 man2/bpf.2 | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)
  

On Fri, Apr 23, 2021 at 4:15 PM Alejandro Colomar
<alx.manpages@gmail.com> wrote:
>
> Some manual pages are already using C99 syntax for integral
> types 'uint32_t', but some aren't.  There are some using kernel
> syntax '__u32'.  Fix those.
>
> Some pages also document attributes, using GNU syntax
> '__attribute__((xxx))'.  Update those to use the shorter and more
> portable C2x syntax, which hasn't been standardized yet, but is
> already implemented in GCC, and available through either --std=c2x
> or any of the --std=gnu... options.
>
> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> ---
>  man2/bpf.2 | 47 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/man2/bpf.2 b/man2/bpf.2
> index 6e1ffa198..204f01bfc 100644
> --- a/man2/bpf.2
> +++ b/man2/bpf.2
> @@ -188,39 +188,38 @@ commands:
>  .EX
>  union bpf_attr {
>      struct {    /* Used by BPF_MAP_CREATE */
> -        __u32         map_type;
> -        __u32         key_size;    /* size of key in bytes */
> -        __u32         value_size;  /* size of value in bytes */
> -        __u32         max_entries; /* maximum number of entries
> -                                      in a map */
> +        uint32_t    map_type;
> +        uint32_t    key_size;    /* size of key in bytes */
> +        uint32_t    value_size;  /* size of value in bytes */
> +        uint32_t    max_entries; /* maximum number of entries
> +                                    in a map */

Nack.
The man page should describe the kernel api the way it is in .h file.
  

Hello Alexei,

On 4/24/21 1:20 AM, Alexei Starovoitov wrote:
> Nack.
> The man page should describe the kernel api the way it is in .h file.

Why?

When glibc uses __size_t (or any other non-standard types) just because 
the standard doesn't allow it to define some types in some specific 
header, the manual pages document the equivalent standard type, (i.e., 
if glibc uses __size_t, we document size_t).

The compiler, AFAIK (gcc is CCd, so they can jump in if I'm wrong), 
using uint32_t in every situation where __u32 is expected.  They're both 
typedefs for the same basic type.

I can understand why Linux will keep using u32 types (and their __ user 
space variants), but that doesn't mean user space programs need to use 
the same type.

If we have a standard syntax for fixed-width integral types (and for 
anything, actually), the manual pages should probably follow it, 
whenever possible.  Any deviation from the standard (be it C or POSIX) 
should have a very good reason to be;  otherwise, it only creates confusion.

Thanks,

Alex
  

From: Alexei Starovoitov
> Sent: 24 April 2021 00:20
> 
> On Fri, Apr 23, 2021 at 4:15 PM Alejandro Colomar
> <alx.manpages@gmail.com> wrote:
> >
> > Some manual pages are already using C99 syntax for integral
> > types 'uint32_t', but some aren't.  There are some using kernel
> > syntax '__u32'.  Fix those.
> >
> > Some pages also document attributes, using GNU syntax
> > '__attribute__((xxx))'.  Update those to use the shorter and more
> > portable C2x syntax, which hasn't been standardized yet, but is
> > already implemented in GCC, and available through either --std=c2x
> > or any of the --std=gnu... options.
> >
> > Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> > ---
> >  man2/bpf.2 | 47 +++++++++++++++++++++++------------------------
> >  1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/man2/bpf.2 b/man2/bpf.2
> > index 6e1ffa198..204f01bfc 100644
> > --- a/man2/bpf.2
> > +++ b/man2/bpf.2
> > @@ -188,39 +188,38 @@ commands:
> >  .EX
> >  union bpf_attr {
> >      struct {    /* Used by BPF_MAP_CREATE */
> > -        __u32         map_type;
> > -        __u32         key_size;    /* size of key in bytes */
> > -        __u32         value_size;  /* size of value in bytes */
> > -        __u32         max_entries; /* maximum number of entries
> > -                                      in a map */
> > +        uint32_t    map_type;
> > +        uint32_t    key_size;    /* size of key in bytes */
> > +        uint32_t    value_size;  /* size of value in bytes */
> > +        uint32_t    max_entries; /* maximum number of entries
> > +                                    in a map */
> 
> Nack.
> The man page should describe the kernel api the way it is in .h file.

And the code below is no more portable that a #pragma'.
It is probably worse than __attribute__((aligned(8)))
+            uint64_t [[gnu::aligned(8)]] value;
The standards committee are smoking dope again.
At least the '__aligned_u64 value;' form stands a reasonable
chance of being converted by cpp into whatever your compiler supports.

OTOH the bfp developers want shooting for defining a structure
with hidden padding fields.
It they ensured that all 64bit fields were aligned they wouldn't
need the __aligned_u64 at all.
And would be much less likely to leak kernel stack to userspace.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Sat, Apr 24, 2021 at 10:56 AM Alejandro Colomar (man-pages)
<alx.manpages@gmail.com> wrote:
>
> Hello Alexei,
>
> On 4/24/21 1:20 AM, Alexei Starovoitov wrote:
> > Nack.
> > The man page should describe the kernel api the way it is in .h file.
>
> Why?

Because man page must describe the linux uapi headers the way they
are installed in the system and not invent alternative implementations.
The users will include those .h with __u32 and will see them in their code.
Man page saying something else is a dangerous lie.

> using uint32_t in every situation where __u32 is expected.  They're both
> typedefs for the same basic type.

That's irrelevant. Languages like golang have their own bpf.h equivalent
that matches /usr/include/linux/bpf.h.

> I can understand why Linux will keep using u32 types (and their __ user
> space variants), but that doesn't mean user space programs need to use
> the same type.

No one says that the users must use __u32. See golang example.
But if the users do #include <linux/bpf.h> they will get them and man page
must describe that.

> If we have a standard syntax for fixed-width integral types (and for
> anything, actually), the manual pages should probably follow it,
> whenever possible.

Absolutely not. linux man page must describe linux.
  

On Sun, Apr 25, 2021 at 12:52 PM Alexei Starovoitov via Libc-alpha
<libc-alpha@sourceware.org> wrote:
> On Sat, Apr 24, 2021 at 10:56 AM Alejandro Colomar (man-pages)
> <alx.manpages@gmail.com> wrote:
> >
> > Hello Alexei,
> >
> > On 4/24/21 1:20 AM, Alexei Starovoitov wrote:
> > > Nack.
> > > The man page should describe the kernel api the way it is in .h file.
> >
> > Why?
>
> Because man page must describe the linux uapi headers the way they
> are installed in the system and not invent alternative implementations.
> The users will include those .h with __u32 and will see them in their code.
> Man page saying something else is a dangerous lie.

Why do you consider it _dangerous_ for the manpages to replace __u32
with uint32_t, when we know by construction that the two types will
always be the same?  Alejandro's preference for the types standardized
by ISO C seems perfectly reasonable to me for documentation; people
reading the documentation can be expected to already know what they
mean, unlike the  Linux-specifc __[iu]NN types.  Also, all else being
equal, documentation should avoid use of symbols in the ISO C reserved
namespace.

If anything I would argue that it is the uapi headers that should be
changed, to use the <stdint.h> types.

zw
  

On Sat, Apr 24, 2021 at 4:43 PM David Laight via Libc-alpha
<libc-alpha@sourceware.org> wrote:
> From: Alexei Starovoitov
> > On Fri, Apr 23, 2021 at 4:15 PM Alejandro Colomar <alx.manpages@gmail.com> wrote:
...
> > > Some pages also document attributes, using GNU syntax
> > > '__attribute__((xxx))'.  Update those to use the shorter and more
> > > portable C2x syntax, which hasn't been standardized yet, but is
> > > already implemented in GCC, and available through either --std=c2x
> > > or any of the --std=gnu... options.
..
> And the code below is no more portable that a #pragma'.
> It is probably worse than __attribute__((aligned(8)))
> +            uint64_t [[gnu::aligned(8)]] value;
> The standards committee are smoking dope again.
> At least the '__aligned_u64 value;' form stands a reasonable
> chance of being converted by cpp into whatever your compiler supports.

Is it actually necessary to mention the alignment overrides at all in
the manpages?  They are only relevant to people working at the level
of physical layout of the data in RAM, and those people are probably
going to have to consult the header file anyway.

zw
  

From: Zack Weinberg
> Sent: 25 April 2021 20:17
> 
> On Sat, Apr 24, 2021 at 4:43 PM David Laight via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> > From: Alexei Starovoitov
> > > On Fri, Apr 23, 2021 at 4:15 PM Alejandro Colomar <alx.manpages@gmail.com> wrote:
> ...
> > > > Some pages also document attributes, using GNU syntax
> > > > '__attribute__((xxx))'.  Update those to use the shorter and more
> > > > portable C2x syntax, which hasn't been standardized yet, but is
> > > > already implemented in GCC, and available through either --std=c2x
> > > > or any of the --std=gnu... options.
> ..
> > And the code below is no more portable that a #pragma'.
> > It is probably worse than __attribute__((aligned(8)))
> > +            uint64_t [[gnu::aligned(8)]] value;
> > The standards committee are smoking dope again.
> > At least the '__aligned_u64 value;' form stands a reasonable
> > chance of being converted by cpp into whatever your compiler supports.
> 
> Is it actually necessary to mention the alignment overrides at all in
> the manpages?  They are only relevant to people working at the level
> of physical layout of the data in RAM, and those people are probably
> going to have to consult the header file anyway.

Depends, if the man page defines the structure - it needs to
contain its definition.
If theory the man page ought to be the definition, and the code
do what the man page says happens.

An alternative is for the man page to say that the structure
contains some fields - without prescribing the order, or
stopping the implementation adding additional fields (or even
changing the actual numeric type).
This is more common in the standards documents.
IMHO The Linux pages really ought to say how linux does things.
(With notes about portability.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Sat, 24 Apr 2021, Alejandro Colomar via Libc-alpha wrote:

> Some pages also document attributes, using GNU syntax
> '__attribute__((xxx))'.  Update those to use the shorter and more
> portable C2x syntax, which hasn't been standardized yet, but is
> already implemented in GCC, and available through either --std=c2x
> or any of the --std=gnu... options.

If you mention alignment in the manpage at all, the same reasoning would 
say you should use _Alignas(8) not [[gnu::aligned(8)]], in any context 
where _Alignas is valid.
  

Hi Joseph,

On 4/26/21 7:19 PM, Joseph Myers wrote:
> On Sat, 24 Apr 2021, Alejandro Colomar via Libc-alpha wrote:
> 
>> Some pages also document attributes, using GNU syntax
>> '__attribute__((xxx))'.  Update those to use the shorter and more
>> portable C2x syntax, which hasn't been standardized yet, but is
>> already implemented in GCC, and available through either --std=c2x
>> or any of the --std=gnu... options.
> 
> If you mention alignment in the manpage at all, the same reasoning would
> say you should use _Alignas(8) not [[gnu::aligned(8)]], in any context
> where _Alignas is valid.
> 

Agree.

I just didn't know 'alignas()' (a.k.a. '_Alignas()'), so I used 
attributes and only changed the syntax.  But yes, we should use that C11 
feature.  Given that we already used 'noreturn' and not '_Noreturn' (see 
exit(3) and its family), I'll use 'alignas()'.

I'll send a v2 with those changes.

Thanks,

Alex