| Message ID | cover.1686135913.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/nolibc: add a new syscall helper | expand |
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@weissschuh.net/ https://lore.kernel.org/all/20230328-nolibc-c99-v1-1-a8302fb19f19@weissschuh.net/ > * 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@tinylab.org/ > [2]: https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git > [3]: https://lore.kernel.org/linux-riscv/cover.1686128703.git.falcon@tinylab.org/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@weissschuh.net> Thanks, Thomas
> 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@weissschuh.net/ > https://lore.kernel.org/all/20230328-nolibc-c99-v1-1-a8302fb19f19@weissschuh.net/ > 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@weissschuh.net> > Thanks a lot, I'm really appreciated. Best regards, Zhangjin > Thanks, > Thomas
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 ? ;-) * 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__) Thanks. Best regards, Zhangjin --- [1]: https://lore.kernel.org/linux-riscv/cover.1686036862.git.falcon@tinylab.org/ [2]: https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git [3]: https://lore.kernel.org/linux-riscv/cover.1686128703.git.falcon@tinylab.org/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(-)