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

[Thomas Weißschuh]

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.
> 
> Changes from v2 -> v3:
> 
> * tools/nolibc: sys.h: add a syscall return helper
> 
>   * The __syscall() is removed.
> 
>   * Align the code style of __sysret() with the others, and use
>     __inline__ instead of inline (like stdlib.h) to let it work with
>     the default -std=c89 in tools/testing/selftests/nolibc/Makefile
> 
> * tools/nolibc: unistd.h: apply __sysret() helper
> 
>   As v2.
> 
> * tools/nolibc: sys.h: apply __sysret() helper
> 
>   replaced __syscall() with __sysret() and merged two separated patches of v2 to one.
> 
> Did run-user tests for rv32 (with [3]), rv64 and arm64.
> 
> 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 :-)

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/

> * 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.

> Thanks.
> 
> Best regards,
> Zhangjin
> 
> ---
> [1]: https://lore.kernel.org/linux-riscv/cover.1686036862.git.falcon@xxxxxxxxxxx/
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git
> [3]: https://lore.kernel.org/linux-riscv/cover.1686128703.git.falcon@xxxxxxxxxxx/T/#t
> 
> Zhangjin Wu (3):
>   tools/nolibc: sys.h: add a syscall return helper
>   tools/nolibc: unistd.h: apply __sysret() helper
>   tools/nolibc: sys.h: apply __sysret() helper
> 
>  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,
Thomas