Re: [PATCH v3 0/3] tools/nolibc: add a new syscall helper

[Zhangjin Wu]

> On 2023-06-07 19:28:58+0800, Zhangjin Wu wrote:
> > Willy, Thomas
> > 
> > This is the revision of the v2 syscall helpers [1], it is based on
> > 20230606-nolibc-rv32+stkp7a of [2]. It doesn't conflict with the v4 of
> > -ENOSYS patchset [3], so, it is ok to simply merge both of them.
> > 
> > This revision mainly applied Thomas' method, removed the __syscall()
> > helper and replaced it with __sysret() instead, because __syscall()
> > looks like _syscall() and syscall(), it may mixlead the developers.
> >
(...)
> > BTW, two questions for Thomas,
> > 
> > * This commit 659a49abc9c2 ("tools/nolibc: validate C89 compatibility")
> >   enables -std=c89, why not gnu11 used by kernel ? ;-)
> 
> Because nolibc needs to support whatever its users need.
> As nolibc is header-only all of it needs to work everywhere.
> C89 should work for everybody :-)
> 

Get it, thanks.

> The kernel on the other hand is compiled standalone and is not limited
> by its users.
> 
> See the discussion here:
> 
> https://lore.kernel.org/all/20230328-nolibc-c99-v2-0-c989f2289222@xxxxxxxxxxxxxx/
> https://lore.kernel.org/all/20230328-nolibc-c99-v1-1-a8302fb19f19@xxxxxxxxxxxxxx/
>

Thanks very much for sharing the whole history info.

And as the your commit 063b6bc5b39f ("tools/nolibc: use __inline__ syntax")
explains, the 'inline' keyword has been used in many headers of include/uapi/,
so, how our -std=c89 work with them? I did find the clue eventually, here maybe:

    $ grep -n inline scripts/headers_install.sh
    11:	echo "asm/inline/volatile keywords."
    37:	s/(^|[[:space:](])(inline|asm|volatile)([[:space:](]|$)/\1__\2__\3/g

The headers_install target helped us convert all of the new keywords to the old
ones, it's magic ;-)

So, it should work if people not want to try a -I/path/to/include/uapi/, I did
this for musl before, even If we do this, this may help:

    diff --git a/tools/include/nolibc/std.h b/tools/include/nolibc/std.h
    index 933bc0be7e1c..33d546cf9af0 100644
    --- a/tools/include/nolibc/std.h
    +++ b/tools/include/nolibc/std.h
    @@ -7,6 +7,14 @@
     #ifndef _NOLIBC_STD_H
     #define _NOLIBC_STD_H
     
    +#ifndef NOLIBC_TEST
    +#ifndef __STDC_VERSION__
    +#define inline __inline__
    +#define asm __asm__
    +#define volatile __volatile__
    +#endif
    +#endif
    +
     /* Declare a few quite common macros and types that usually are in stdlib.h,
      * stdint.h, ctype.h, unistd.h and a few other common locations. Please place
      * integer type definitions and generic macros here, but avoid OS-specific and
    diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
    index 4a3a105e1fdf..46f061a4458a 100644
    --- a/tools/testing/selftests/nolibc/Makefile
    +++ b/tools/testing/selftests/nolibc/Makefile
    @@ -78,7 +78,7 @@ endif

     CFLAGS_s390 = -m64
     CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
    -CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
    +CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 -DNOLIBC_TEST \
                    $(call cc-option,-fno-stack-protector) \
                    $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)

Is this worth a new patch? I do think it is not required.

> > * Do we need to tune the order of the macros in unistd.h like this:
> > 
> >     #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))
> >     #define _syscall_n(N, ...) _syscall(N, __VA_ARGS__)
> >     #define __syscall_narg(_0, _1, _2, _3, _4, _5, _6, N, ...) N
> >     #define _sycall_narg(...) __syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0)
> >     #define syscall(...) _syscall_n(_sycall_narg(__VA_ARGS__), ##__VA_ARGS__)
> > 
> >   Before, It works but seems not put in using order:
> > 
> >     #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))
> >     #define _sycall_narg(...) __syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0)
> >     #define __syscall_narg(_0, _1, _2, _3, _4, _5, _6, N, ...) N
> >     #define _syscall_n(N, ...) _syscall(N, __VA_ARGS__)
> >     #define syscall(...) _syscall_n(_sycall_narg(__VA_ARGS__), ##__VA_ARGS__)
> 
> Not sure it makes a big difference.
> If you want to change it, go for it.
>

Only switched two of them, oh, just found the '_sycall_narg' did miss a 's'
character, it may be really worth a patch now, I know why I focused on the
order so much, because the missing 's' made it not aligned well ;-)
 
> >
(...) 
> >  tools/include/nolibc/sys.h    | 364 +++++-----------------------------
> >  tools/include/nolibc/unistd.h |  11 +-
> >  2 files changed, 55 insertions(+), 320 deletions(-)
> 
> For the full series:
> 
> Reviewed-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
>

Thanks a lot, I'm really appreciated.

Best regards,
Zhangjin
 
> Thanks,
> Thomas