[PATCH 0/4] tools/nolibc: add two new syscall helpers

[PATCH 0/4] tools/nolibc: add two new syscall helpers

[Zhangjin Wu]

Hi, Willy

When I worked on adding new syscalls and the related library routines,
I have seen most of the library routines share the same syscall call and
return logic, this patchset adds two macros to simplify and shrink them.

All of them have been tested on arm, aarch64, rv32 and rv64, no new
regressions found.

If this is ok, I will rebase the new syscalls and library routines on
this patchset.

Best regards,
Zhangjin
---

Zhangjin Wu (4):
  tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers
  tools/nolibc: unistd.h: apply __syscall_ret() helper
  tools/nolibc: sys.h: apply __syscall_ret() helper
  tools/nolibc: sys.h: apply __syscall() helper

 tools/include/nolibc/sys.h    | 369 ++++++----------------------------
 tools/include/nolibc/unistd.h |  12 +-
 2 files changed, 65 insertions(+), 316 deletions(-)

-- 
2.25.1

[PATCH 0/4] tools/nolibc: add two new syscall helpers

[Zhangjin Wu]

Hi, Willy

When I worked on adding new syscalls and the related library routines,
I have seen most of the library routines share the same syscall call and
return logic, this patchset adds two macros to simplify and shrink them.

All of them have been tested on arm, aarch64, rv32 and rv64, no new
regressions found.

If this is ok, I will rebase the new syscalls and library routines on
this patchset.

Best regards,
Zhangjin
---

Zhangjin Wu (4):
  tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers
  tools/nolibc: unistd.h: apply __syscall_ret() helper
  tools/nolibc: sys.h: apply __syscall_ret() helper
  tools/nolibc: sys.h: apply __syscall() helper

 tools/include/nolibc/sys.h    | 369 ++++++----------------------------
 tools/include/nolibc/unistd.h |  12 +-
 2 files changed, 65 insertions(+), 316 deletions(-)

-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Zhangjin Wu]

most of the library routines share the same code model, let's add some
macros to simplify the coding and shrink the code lines too.

One added for syscall return, one added for syscall call, both of them
can get the typeof 'return value' automatically.

To get the return type of syscalls, __auto_type is better than typeof(),
but it is not supported by the old compilers (before 2013, see [1]), so,
use typeof() here.

[1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 1d6f33f58629..937a8578e3d4 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -28,6 +28,21 @@
 #include "errno.h"
 #include "types.h"
 
+/* Syscall call and return helpers */
+#define __syscall_ret(ret)						\
+({									\
+	if (ret < 0) {							\
+		SET_ERRNO(-ret);					\
+		ret = (typeof(ret))-1;					\
+	}								\
+	ret;								\
+})
+
+#define __syscall(name, ...)						\
+({									\
+	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
+	__syscall_ret(ret);						\
+})
 
 /* Functions in this file only describe syscalls. They're declared static so
  * that the compiler usually decides to inline them while still being allowed
-- 
2.25.1

[PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Zhangjin Wu]

most of the library routines share the same code model, let's add some
macros to simplify the coding and shrink the code lines too.

One added for syscall return, one added for syscall call, both of them
can get the typeof 'return value' automatically.

To get the return type of syscalls, __auto_type is better than typeof(),
but it is not supported by the old compilers (before 2013, see [1]), so,
use typeof() here.

[1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 1d6f33f58629..937a8578e3d4 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -28,6 +28,21 @@
 #include "errno.h"
 #include "types.h"
 
+/* Syscall call and return helpers */
+#define __syscall_ret(ret)						\
+({									\
+	if (ret < 0) {							\
+		SET_ERRNO(-ret);					\
+		ret = (typeof(ret))-1;					\
+	}								\
+	ret;								\
+})
+
+#define __syscall(name, ...)						\
+({									\
+	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
+	__syscall_ret(ret);						\
+})
 
 /* Functions in this file only describe syscalls. They're declared static so
  * that the compiler usually decides to inline them while still being allowed
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/4] tools/nolibc: unistd.h: apply __syscall_ret() helper

[Zhangjin Wu]

get the return type automatically and use __syscall_ret() to shrink 4
code lines.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/unistd.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tools/include/nolibc/unistd.h b/tools/include/nolibc/unistd.h
index 6773e83c16a0..e4e2b4c09771 100644
--- a/tools/include/nolibc/unistd.h
+++ b/tools/include/nolibc/unistd.h
@@ -56,14 +56,10 @@ int tcsetpgrp(int fd, pid_t pid)
 	return ioctl(fd, TIOCSPGRP, &pid);
 }
 
-#define _syscall(N, ...)                                                      \
-({                                                                            \
-	int _ret = my_syscall##N(__VA_ARGS__);                                \
-	if (_ret < 0) {                                                       \
-		SET_ERRNO(-_ret);                                             \
-		_ret = -1;                                                    \
-	}                                                                     \
-	_ret;                                                                 \
+#define _syscall(N, ...)							\
+({										\
+	typeof(my_syscall##N(__VA_ARGS__)) _ret = my_syscall##N(__VA_ARGS__);	\
+	__syscall_ret(_ret);							\
 })
 
 #define _sycall_narg(...) __syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0)
-- 
2.25.1

[PATCH 2/4] tools/nolibc: unistd.h: apply __syscall_ret() helper

[Zhangjin Wu]

get the return type automatically and use __syscall_ret() to shrink 4
code lines.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/unistd.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tools/include/nolibc/unistd.h b/tools/include/nolibc/unistd.h
index 6773e83c16a0..e4e2b4c09771 100644
--- a/tools/include/nolibc/unistd.h
+++ b/tools/include/nolibc/unistd.h
@@ -56,14 +56,10 @@ int tcsetpgrp(int fd, pid_t pid)
 	return ioctl(fd, TIOCSPGRP, &pid);
 }
 
-#define _syscall(N, ...)                                                      \
-({                                                                            \
-	int _ret = my_syscall##N(__VA_ARGS__);                                \
-	if (_ret < 0) {                                                       \
-		SET_ERRNO(-_ret);                                             \
-		_ret = -1;                                                    \
-	}                                                                     \
-	_ret;                                                                 \
+#define _syscall(N, ...)							\
+({										\
+	typeof(my_syscall##N(__VA_ARGS__)) _ret = my_syscall##N(__VA_ARGS__);	\
+	__syscall_ret(_ret);							\
 })
 
 #define _sycall_narg(...) __syscall_narg(__VA_ARGS__, 6, 5, 4, 3, 2, 1, 0)
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/4] tools/nolibc: sys.h: apply __syscall_ret() helper

[Zhangjin Wu]

Use __syscall_ret() helper to shrink the code lines of brk() and
getpagesize(), 10 lines removed.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 937a8578e3d4..976f23d1fdad 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -81,13 +81,9 @@ void *sys_brk(void *addr)
 static __attribute__((unused))
 int brk(void *addr)
 {
-	void *ret = sys_brk(addr);
+	int ret = sys_brk(addr) ? 0 : -ENOMEM;
 
-	if (!ret) {
-		SET_ERRNO(ENOMEM);
-		return -1;
-	}
-	return 0;
+	return __syscall_ret(ret);
 }
 
 static __attribute__((unused))
@@ -550,15 +546,9 @@ static unsigned long getauxval(unsigned long key);
 static __attribute__((unused))
 long getpagesize(void)
 {
-	long ret;
+	long ret = getauxval(AT_PAGESZ) ?: -ENOENT;
 
-	ret = getauxval(AT_PAGESZ);
-	if (!ret) {
-		SET_ERRNO(ENOENT);
-		return -1;
-	}
-
-	return ret;
+	return __syscall_ret(ret);
 }
 
 
-- 
2.25.1

[PATCH 3/4] tools/nolibc: sys.h: apply __syscall_ret() helper

[Zhangjin Wu]

Use __syscall_ret() helper to shrink the code lines of brk() and
getpagesize(), 10 lines removed.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 937a8578e3d4..976f23d1fdad 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -81,13 +81,9 @@ void *sys_brk(void *addr)
 static __attribute__((unused))
 int brk(void *addr)
 {
-	void *ret = sys_brk(addr);
+	int ret = sys_brk(addr) ? 0 : -ENOMEM;
 
-	if (!ret) {
-		SET_ERRNO(ENOMEM);
-		return -1;
-	}
-	return 0;
+	return __syscall_ret(ret);
 }
 
 static __attribute__((unused))
@@ -550,15 +546,9 @@ static unsigned long getauxval(unsigned long key);
 static __attribute__((unused))
 long getpagesize(void)
 {
-	long ret;
+	long ret = getauxval(AT_PAGESZ) ?: -ENOENT;
 
-	ret = getauxval(AT_PAGESZ);
-	if (!ret) {
-		SET_ERRNO(ENOENT);
-		return -1;
-	}
-
-	return ret;
+	return __syscall_ret(ret);
 }
 
 
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 4/4] tools/nolibc: sys.h: apply __syscall() helper

[Zhangjin Wu]

Use __syscall() helper to shrink 252 lines of code.

    $ git show HEAD^:tools/include/nolibc/sys.h | wc -l
    1432
    $ git show HEAD:tools/include/nolibc/sys.h | wc -l
    1180
    $ echo "1432-1180" | bc -l
    252

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 336 +++++--------------------------------
 1 file changed, 42 insertions(+), 294 deletions(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 976f23d1fdad..1f64f760d7fd 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -113,13 +113,7 @@ int sys_chdir(const char *path)
 static __attribute__((unused))
 int chdir(const char *path)
 {
-	int ret = sys_chdir(path);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chdir, path);
 }
 
 
@@ -142,13 +136,7 @@ int sys_chmod(const char *path, mode_t mode)
 static __attribute__((unused))
 int chmod(const char *path, mode_t mode)
 {
-	int ret = sys_chmod(path, mode);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chmod, path, mode);
 }
 
 
@@ -171,13 +159,7 @@ int sys_chown(const char *path, uid_t owner, gid_t group)
 static __attribute__((unused))
 int chown(const char *path, uid_t owner, gid_t group)
 {
-	int ret = sys_chown(path, owner, group);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chown, path, owner, group);
 }
 
 
@@ -194,13 +176,7 @@ int sys_chroot(const char *path)
 static __attribute__((unused))
 int chroot(const char *path)
 {
-	int ret = sys_chroot(path);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chroot, path);
 }
 
 
@@ -217,13 +193,7 @@ int sys_close(int fd)
 static __attribute__((unused))
 int close(int fd)
 {
-	int ret = sys_close(fd);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(close, fd);
 }
 
 
@@ -240,13 +210,7 @@ int sys_dup(int fd)
 static __attribute__((unused))
 int dup(int fd)
 {
-	int ret = sys_dup(fd);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(dup, fd);
 }
 
 
@@ -269,13 +233,7 @@ int sys_dup2(int old, int new)
 static __attribute__((unused))
 int dup2(int old, int new)
 {
-	int ret = sys_dup2(old, new);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(dup2, old, new);
 }
 
 
@@ -293,13 +251,7 @@ int sys_dup3(int old, int new, int flags)
 static __attribute__((unused))
 int dup3(int old, int new, int flags)
 {
-	int ret = sys_dup3(old, new, flags);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(dup3, old, new, flags);
 }
 #endif
 
@@ -317,13 +269,7 @@ int sys_execve(const char *filename, char *const argv[], char *const envp[])
 static __attribute__((unused))
 int execve(const char *filename, char *const argv[], char *const envp[])
 {
-	int ret = sys_execve(filename, argv, envp);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(execve, filename, argv, envp);
 }
 
 
@@ -370,13 +316,7 @@ pid_t sys_fork(void)
 static __attribute__((unused))
 pid_t fork(void)
 {
-	pid_t ret = sys_fork();
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(fork);
 }
 
 
@@ -393,13 +333,7 @@ int sys_fsync(int fd)
 static __attribute__((unused))
 int fsync(int fd)
 {
-	int ret = sys_fsync(fd);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(fsync, fd);
 }
 
 
@@ -416,13 +350,7 @@ int sys_getdents64(int fd, struct linux_dirent64 *dirp, int count)
 static __attribute__((unused))
 int getdents64(int fd, struct linux_dirent64 *dirp, int count)
 {
-	int ret = sys_getdents64(fd, dirp, count);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(getdents64, fd, dirp, count);
 }
 
 
@@ -460,13 +388,7 @@ pid_t sys_getpgid(pid_t pid)
 static __attribute__((unused))
 pid_t getpgid(pid_t pid)
 {
-	pid_t ret = sys_getpgid(pid);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(getpgid, pid);
 }
 
 
@@ -565,13 +487,7 @@ int sys_gettimeofday(struct timeval *tv, struct timezone *tz)
 static __attribute__((unused))
 int gettimeofday(struct timeval *tv, struct timezone *tz)
 {
-	int ret = sys_gettimeofday(tv, tz);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(gettimeofday, tv, tz);
 }
 
 
@@ -609,13 +525,7 @@ int sys_ioctl(int fd, unsigned long req, void *value)
 static __attribute__((unused))
 int ioctl(int fd, unsigned long req, void *value)
 {
-	int ret = sys_ioctl(fd, req, value);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(ioctl, fd, req, value);
 }
 
 /*
@@ -631,13 +541,7 @@ int sys_kill(pid_t pid, int signal)
 static __attribute__((unused))
 int kill(pid_t pid, int signal)
 {
-	int ret = sys_kill(pid, signal);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(kill, pid, signal);
 }
 
 
@@ -660,13 +564,7 @@ int sys_link(const char *old, const char *new)
 static __attribute__((unused))
 int link(const char *old, const char *new)
 {
-	int ret = sys_link(old, new);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(link, old, new);
 }
 
 
@@ -683,13 +581,7 @@ off_t sys_lseek(int fd, off_t offset, int whence)
 static __attribute__((unused))
 off_t lseek(int fd, off_t offset, int whence)
 {
-	off_t ret = sys_lseek(fd, offset, whence);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(lseek, fd, offset, whence);
 }
 
 
@@ -712,13 +604,7 @@ int sys_mkdir(const char *path, mode_t mode)
 static __attribute__((unused))
 int mkdir(const char *path, mode_t mode)
 {
-	int ret = sys_mkdir(path, mode);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(mkdir, path, mode);
 }
 
 
@@ -741,13 +627,7 @@ long sys_mknod(const char *path, mode_t mode, dev_t dev)
 static __attribute__((unused))
 int mknod(const char *path, mode_t mode, dev_t dev)
 {
-	int ret = sys_mknod(path, mode, dev);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(mknod, path, mode, dev);
 }
 
 #ifndef MAP_SHARED
@@ -805,13 +685,7 @@ int sys_munmap(void *addr, size_t length)
 static __attribute__((unused))
 int munmap(void *addr, size_t length)
 {
-	int ret = sys_munmap(addr, length);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(munmap, addr, length);
 }
 
 /*
@@ -831,13 +705,7 @@ int mount(const char *src, const char *tgt,
           const char *fst, unsigned long flags,
           const void *data)
 {
-	int ret = sys_mount(src, tgt, fst, flags, data);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(mount, src, tgt, fst, flags, data);
 }
 
 
@@ -871,13 +739,7 @@ int open(const char *path, int flags, ...)
 		va_end(args);
 	}
 
-	ret = sys_open(path, flags, mode);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(open, path, flags, mode);
 }
 
 
@@ -897,13 +759,7 @@ static __attribute__((unused))
 int prctl(int option, unsigned long arg2, unsigned long arg3,
 		      unsigned long arg4, unsigned long arg5)
 {
-	int ret = sys_prctl(option, arg2, arg3, arg4, arg5);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(prctl, option, arg2, arg3, arg4, arg5);
 }
 
 
@@ -920,13 +776,7 @@ int sys_pivot_root(const char *new, const char *old)
 static __attribute__((unused))
 int pivot_root(const char *new, const char *old)
 {
-	int ret = sys_pivot_root(new, old);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(pivot_root, new, old);
 }
 
 
@@ -955,13 +805,7 @@ int sys_poll(struct pollfd *fds, int nfds, int timeout)
 static __attribute__((unused))
 int poll(struct pollfd *fds, int nfds, int timeout)
 {
-	int ret = sys_poll(fds, nfds, timeout);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(poll, fds, nfds, timeout);
 }
 
 
@@ -978,13 +822,7 @@ ssize_t sys_read(int fd, void *buf, size_t count)
 static __attribute__((unused))
 ssize_t read(int fd, void *buf, size_t count)
 {
-	ssize_t ret = sys_read(fd, buf, count);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(read, fd, buf, count);
 }
 
 
@@ -1002,13 +840,7 @@ ssize_t sys_reboot(int magic1, int magic2, int cmd, void *arg)
 static __attribute__((unused))
 int reboot(int cmd)
 {
-	int ret = sys_reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, cmd, 0);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, cmd, 0);
 }
 
 
@@ -1025,13 +857,7 @@ int sys_sched_yield(void)
 static __attribute__((unused))
 int sched_yield(void)
 {
-	int ret = sys_sched_yield();
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(sched_yield);
 }
 
 
@@ -1071,13 +897,7 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva
 static __attribute__((unused))
 int select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeval *timeout)
 {
-	int ret = sys_select(nfds, rfds, wfds, efds, timeout);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(select, nfds, rfds, wfds, efds, timeout);
 }
 
 
@@ -1094,13 +914,7 @@ int sys_setpgid(pid_t pid, pid_t pgid)
 static __attribute__((unused))
 int setpgid(pid_t pid, pid_t pgid)
 {
-	int ret = sys_setpgid(pid, pgid);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(setpgid, pid, pgid);
 }
 
 
@@ -1117,13 +931,7 @@ pid_t sys_setsid(void)
 static __attribute__((unused))
 pid_t setsid(void)
 {
-	pid_t ret = sys_setsid();
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(setsid);
 }
 
 #if defined(__NR_statx)
@@ -1140,13 +948,7 @@ int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct sta
 static __attribute__((unused))
 int statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf)
 {
-	int ret = sys_statx(fd, path, flags, mask, buf);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(statx, fd, path, flags, mask, buf);
 }
 #endif
 
@@ -1223,13 +1025,7 @@ int sys_stat(const char *path, struct stat *buf)
 static __attribute__((unused))
 int stat(const char *path, struct stat *buf)
 {
-	int ret = sys_stat(path, buf);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(stat, path, buf);
 }
 
 
@@ -1252,13 +1048,7 @@ int sys_symlink(const char *old, const char *new)
 static __attribute__((unused))
 int symlink(const char *old, const char *new)
 {
-	int ret = sys_symlink(old, new);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(symlink, old, new);
 }
 
 
@@ -1292,13 +1082,7 @@ int sys_umount2(const char *path, int flags)
 static __attribute__((unused))
 int umount2(const char *path, int flags)
 {
-	int ret = sys_umount2(path, flags);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(umount2, path, flags);
 }
 
 
@@ -1321,13 +1105,7 @@ int sys_unlink(const char *path)
 static __attribute__((unused))
 int unlink(const char *path)
 {
-	int ret = sys_unlink(path);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(unlink, path);
 }
 
 
@@ -1346,38 +1124,20 @@ pid_t sys_wait4(pid_t pid, int *status, int options, struct rusage *rusage)
 static __attribute__((unused))
 pid_t wait(int *status)
 {
-	pid_t ret = sys_wait4(-1, status, 0, NULL);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(wait4, -1, status, 0, NULL);
 }
 
 static __attribute__((unused))
 pid_t wait4(pid_t pid, int *status, int options, struct rusage *rusage)
 {
-	pid_t ret = sys_wait4(pid, status, options, rusage);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(wait4, pid, status, options, rusage);
 }
 
 
 static __attribute__((unused))
 pid_t waitpid(pid_t pid, int *status, int options)
 {
-	pid_t ret = sys_wait4(pid, status, options, NULL);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(wait4, pid, status, options, NULL);
 }
 
 
@@ -1394,13 +1154,7 @@ ssize_t sys_write(int fd, const void *buf, size_t count)
 static __attribute__((unused))
 ssize_t write(int fd, const void *buf, size_t count)
 {
-	ssize_t ret = sys_write(fd, buf, count);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(write, fd, buf, count);
 }
 
 
@@ -1417,13 +1171,7 @@ int sys_memfd_create(const char *name, unsigned int flags)
 static __attribute__((unused))
 int memfd_create(const char *name, unsigned int flags)
 {
-	ssize_t ret = sys_memfd_create(name, flags);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(memfd_create, name, flags);
 }
 
 /* make sure to include all global symbols */
-- 
2.25.1

[PATCH 4/4] tools/nolibc: sys.h: apply __syscall() helper

[Zhangjin Wu]

Use __syscall() helper to shrink 252 lines of code.

    $ git show HEAD^:tools/include/nolibc/sys.h | wc -l
    1432
    $ git show HEAD:tools/include/nolibc/sys.h | wc -l
    1180
    $ echo "1432-1180" | bc -l
    252

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/sys.h | 336 +++++--------------------------------
 1 file changed, 42 insertions(+), 294 deletions(-)

diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 976f23d1fdad..1f64f760d7fd 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -113,13 +113,7 @@ int sys_chdir(const char *path)
 static __attribute__((unused))
 int chdir(const char *path)
 {
-	int ret = sys_chdir(path);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chdir, path);
 }
 
 
@@ -142,13 +136,7 @@ int sys_chmod(const char *path, mode_t mode)
 static __attribute__((unused))
 int chmod(const char *path, mode_t mode)
 {
-	int ret = sys_chmod(path, mode);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chmod, path, mode);
 }
 
 
@@ -171,13 +159,7 @@ int sys_chown(const char *path, uid_t owner, gid_t group)
 static __attribute__((unused))
 int chown(const char *path, uid_t owner, gid_t group)
 {
-	int ret = sys_chown(path, owner, group);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chown, path, owner, group);
 }
 
 
@@ -194,13 +176,7 @@ int sys_chroot(const char *path)
 static __attribute__((unused))
 int chroot(const char *path)
 {
-	int ret = sys_chroot(path);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(chroot, path);
 }
 
 
@@ -217,13 +193,7 @@ int sys_close(int fd)
 static __attribute__((unused))
 int close(int fd)
 {
-	int ret = sys_close(fd);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(close, fd);
 }
 
 
@@ -240,13 +210,7 @@ int sys_dup(int fd)
 static __attribute__((unused))
 int dup(int fd)
 {
-	int ret = sys_dup(fd);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(dup, fd);
 }
 
 
@@ -269,13 +233,7 @@ int sys_dup2(int old, int new)
 static __attribute__((unused))
 int dup2(int old, int new)
 {
-	int ret = sys_dup2(old, new);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(dup2, old, new);
 }
 
 
@@ -293,13 +251,7 @@ int sys_dup3(int old, int new, int flags)
 static __attribute__((unused))
 int dup3(int old, int new, int flags)
 {
-	int ret = sys_dup3(old, new, flags);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(dup3, old, new, flags);
 }
 #endif
 
@@ -317,13 +269,7 @@ int sys_execve(const char *filename, char *const argv[], char *const envp[])
 static __attribute__((unused))
 int execve(const char *filename, char *const argv[], char *const envp[])
 {
-	int ret = sys_execve(filename, argv, envp);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(execve, filename, argv, envp);
 }
 
 
@@ -370,13 +316,7 @@ pid_t sys_fork(void)
 static __attribute__((unused))
 pid_t fork(void)
 {
-	pid_t ret = sys_fork();
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(fork);
 }
 
 
@@ -393,13 +333,7 @@ int sys_fsync(int fd)
 static __attribute__((unused))
 int fsync(int fd)
 {
-	int ret = sys_fsync(fd);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(fsync, fd);
 }
 
 
@@ -416,13 +350,7 @@ int sys_getdents64(int fd, struct linux_dirent64 *dirp, int count)
 static __attribute__((unused))
 int getdents64(int fd, struct linux_dirent64 *dirp, int count)
 {
-	int ret = sys_getdents64(fd, dirp, count);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(getdents64, fd, dirp, count);
 }
 
 
@@ -460,13 +388,7 @@ pid_t sys_getpgid(pid_t pid)
 static __attribute__((unused))
 pid_t getpgid(pid_t pid)
 {
-	pid_t ret = sys_getpgid(pid);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(getpgid, pid);
 }
 
 
@@ -565,13 +487,7 @@ int sys_gettimeofday(struct timeval *tv, struct timezone *tz)
 static __attribute__((unused))
 int gettimeofday(struct timeval *tv, struct timezone *tz)
 {
-	int ret = sys_gettimeofday(tv, tz);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(gettimeofday, tv, tz);
 }
 
 
@@ -609,13 +525,7 @@ int sys_ioctl(int fd, unsigned long req, void *value)
 static __attribute__((unused))
 int ioctl(int fd, unsigned long req, void *value)
 {
-	int ret = sys_ioctl(fd, req, value);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(ioctl, fd, req, value);
 }
 
 /*
@@ -631,13 +541,7 @@ int sys_kill(pid_t pid, int signal)
 static __attribute__((unused))
 int kill(pid_t pid, int signal)
 {
-	int ret = sys_kill(pid, signal);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(kill, pid, signal);
 }
 
 
@@ -660,13 +564,7 @@ int sys_link(const char *old, const char *new)
 static __attribute__((unused))
 int link(const char *old, const char *new)
 {
-	int ret = sys_link(old, new);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(link, old, new);
 }
 
 
@@ -683,13 +581,7 @@ off_t sys_lseek(int fd, off_t offset, int whence)
 static __attribute__((unused))
 off_t lseek(int fd, off_t offset, int whence)
 {
-	off_t ret = sys_lseek(fd, offset, whence);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(lseek, fd, offset, whence);
 }
 
 
@@ -712,13 +604,7 @@ int sys_mkdir(const char *path, mode_t mode)
 static __attribute__((unused))
 int mkdir(const char *path, mode_t mode)
 {
-	int ret = sys_mkdir(path, mode);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(mkdir, path, mode);
 }
 
 
@@ -741,13 +627,7 @@ long sys_mknod(const char *path, mode_t mode, dev_t dev)
 static __attribute__((unused))
 int mknod(const char *path, mode_t mode, dev_t dev)
 {
-	int ret = sys_mknod(path, mode, dev);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(mknod, path, mode, dev);
 }
 
 #ifndef MAP_SHARED
@@ -805,13 +685,7 @@ int sys_munmap(void *addr, size_t length)
 static __attribute__((unused))
 int munmap(void *addr, size_t length)
 {
-	int ret = sys_munmap(addr, length);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(munmap, addr, length);
 }
 
 /*
@@ -831,13 +705,7 @@ int mount(const char *src, const char *tgt,
           const char *fst, unsigned long flags,
           const void *data)
 {
-	int ret = sys_mount(src, tgt, fst, flags, data);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(mount, src, tgt, fst, flags, data);
 }
 
 
@@ -871,13 +739,7 @@ int open(const char *path, int flags, ...)
 		va_end(args);
 	}
 
-	ret = sys_open(path, flags, mode);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(open, path, flags, mode);
 }
 
 
@@ -897,13 +759,7 @@ static __attribute__((unused))
 int prctl(int option, unsigned long arg2, unsigned long arg3,
 		      unsigned long arg4, unsigned long arg5)
 {
-	int ret = sys_prctl(option, arg2, arg3, arg4, arg5);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(prctl, option, arg2, arg3, arg4, arg5);
 }
 
 
@@ -920,13 +776,7 @@ int sys_pivot_root(const char *new, const char *old)
 static __attribute__((unused))
 int pivot_root(const char *new, const char *old)
 {
-	int ret = sys_pivot_root(new, old);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(pivot_root, new, old);
 }
 
 
@@ -955,13 +805,7 @@ int sys_poll(struct pollfd *fds, int nfds, int timeout)
 static __attribute__((unused))
 int poll(struct pollfd *fds, int nfds, int timeout)
 {
-	int ret = sys_poll(fds, nfds, timeout);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(poll, fds, nfds, timeout);
 }
 
 
@@ -978,13 +822,7 @@ ssize_t sys_read(int fd, void *buf, size_t count)
 static __attribute__((unused))
 ssize_t read(int fd, void *buf, size_t count)
 {
-	ssize_t ret = sys_read(fd, buf, count);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(read, fd, buf, count);
 }
 
 
@@ -1002,13 +840,7 @@ ssize_t sys_reboot(int magic1, int magic2, int cmd, void *arg)
 static __attribute__((unused))
 int reboot(int cmd)
 {
-	int ret = sys_reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, cmd, 0);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, cmd, 0);
 }
 
 
@@ -1025,13 +857,7 @@ int sys_sched_yield(void)
 static __attribute__((unused))
 int sched_yield(void)
 {
-	int ret = sys_sched_yield();
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(sched_yield);
 }
 
 
@@ -1071,13 +897,7 @@ int sys_select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeva
 static __attribute__((unused))
 int select(int nfds, fd_set *rfds, fd_set *wfds, fd_set *efds, struct timeval *timeout)
 {
-	int ret = sys_select(nfds, rfds, wfds, efds, timeout);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(select, nfds, rfds, wfds, efds, timeout);
 }
 
 
@@ -1094,13 +914,7 @@ int sys_setpgid(pid_t pid, pid_t pgid)
 static __attribute__((unused))
 int setpgid(pid_t pid, pid_t pgid)
 {
-	int ret = sys_setpgid(pid, pgid);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(setpgid, pid, pgid);
 }
 
 
@@ -1117,13 +931,7 @@ pid_t sys_setsid(void)
 static __attribute__((unused))
 pid_t setsid(void)
 {
-	pid_t ret = sys_setsid();
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(setsid);
 }
 
 #if defined(__NR_statx)
@@ -1140,13 +948,7 @@ int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct sta
 static __attribute__((unused))
 int statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf)
 {
-	int ret = sys_statx(fd, path, flags, mask, buf);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(statx, fd, path, flags, mask, buf);
 }
 #endif
 
@@ -1223,13 +1025,7 @@ int sys_stat(const char *path, struct stat *buf)
 static __attribute__((unused))
 int stat(const char *path, struct stat *buf)
 {
-	int ret = sys_stat(path, buf);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(stat, path, buf);
 }
 
 
@@ -1252,13 +1048,7 @@ int sys_symlink(const char *old, const char *new)
 static __attribute__((unused))
 int symlink(const char *old, const char *new)
 {
-	int ret = sys_symlink(old, new);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(symlink, old, new);
 }
 
 
@@ -1292,13 +1082,7 @@ int sys_umount2(const char *path, int flags)
 static __attribute__((unused))
 int umount2(const char *path, int flags)
 {
-	int ret = sys_umount2(path, flags);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(umount2, path, flags);
 }
 
 
@@ -1321,13 +1105,7 @@ int sys_unlink(const char *path)
 static __attribute__((unused))
 int unlink(const char *path)
 {
-	int ret = sys_unlink(path);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(unlink, path);
 }
 
 
@@ -1346,38 +1124,20 @@ pid_t sys_wait4(pid_t pid, int *status, int options, struct rusage *rusage)
 static __attribute__((unused))
 pid_t wait(int *status)
 {
-	pid_t ret = sys_wait4(-1, status, 0, NULL);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(wait4, -1, status, 0, NULL);
 }
 
 static __attribute__((unused))
 pid_t wait4(pid_t pid, int *status, int options, struct rusage *rusage)
 {
-	pid_t ret = sys_wait4(pid, status, options, rusage);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(wait4, pid, status, options, rusage);
 }
 
 
 static __attribute__((unused))
 pid_t waitpid(pid_t pid, int *status, int options)
 {
-	pid_t ret = sys_wait4(pid, status, options, NULL);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(wait4, pid, status, options, NULL);
 }
 
 
@@ -1394,13 +1154,7 @@ ssize_t sys_write(int fd, const void *buf, size_t count)
 static __attribute__((unused))
 ssize_t write(int fd, const void *buf, size_t count)
 {
-	ssize_t ret = sys_write(fd, buf, count);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(write, fd, buf, count);
 }
 
 
@@ -1417,13 +1171,7 @@ int sys_memfd_create(const char *name, unsigned int flags)
 static __attribute__((unused))
 int memfd_create(const char *name, unsigned int flags)
 {
-	ssize_t ret = sys_memfd_create(name, flags);
-
-	if (ret < 0) {
-		SET_ERRNO(-ret);
-		ret = -1;
-	}
-	return ret;
+	return __syscall(memfd_create, name, flags);
 }
 
 /* make sure to include all global symbols */
-- 
2.25.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Willy Tarreau]

Hi Zhangjin,

On Sun, Jun 04, 2023 at 01:34:29PM +0800, Zhangjin Wu wrote:
> most of the library routines share the same code model, let's add some
> macros to simplify the coding and shrink the code lines too.
> 
> One added for syscall return, one added for syscall call, both of them
> can get the typeof 'return value' automatically.
> 
> To get the return type of syscalls, __auto_type is better than typeof(),
> but it is not supported by the old compilers (before 2013, see [1]), so,
> use typeof() here.
> 
> [1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html
> 
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/include/nolibc/sys.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 1d6f33f58629..937a8578e3d4 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -28,6 +28,21 @@
>  #include "errno.h"
>  #include "types.h"
>  
> +/* Syscall call and return helpers */
> +#define __syscall_ret(ret)						\
> +({									\
> +	if (ret < 0) {							\
> +		SET_ERRNO(-ret);					\
> +		ret = (typeof(ret))-1;					\
> +	}								\
> +	ret;								\
> +})
> +
> +#define __syscall(name, ...)						\
> +({									\
> +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> +	__syscall_ret(ret);						\
> +})

Well, I personally don't find that it increases legibility, on the
opposite. At first when reading the series, I thought you had dropped
errno setting on return. I think the reason is that when reading that
last macro, it's not at all obvious that __syscall_ret() is actually
modifying this ret value *and* returning it as the macro's result.

If we'd want to go down that route, I suspect that something like this
would at least hint about what is being returned:

+#define __syscall(name, ...)						\
+({									\
+	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
+	ret = __syscall_ret(ret);					\
+})

But I'm interested in others' opinion on this, particularly Thomas and
Arnd who review a significant number of patches. For now I prefer not
to take it before we've settled on a choice.

Thanks,
Willy

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Willy Tarreau]

Hi Zhangjin,

On Sun, Jun 04, 2023 at 01:34:29PM +0800, Zhangjin Wu wrote:
> most of the library routines share the same code model, let's add some
> macros to simplify the coding and shrink the code lines too.
> 
> One added for syscall return, one added for syscall call, both of them
> can get the typeof 'return value' automatically.
> 
> To get the return type of syscalls, __auto_type is better than typeof(),
> but it is not supported by the old compilers (before 2013, see [1]), so,
> use typeof() here.
> 
> [1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html
> 
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/include/nolibc/sys.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 1d6f33f58629..937a8578e3d4 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -28,6 +28,21 @@
>  #include "errno.h"
>  #include "types.h"
>  
> +/* Syscall call and return helpers */
> +#define __syscall_ret(ret)						\
> +({									\
> +	if (ret < 0) {							\
> +		SET_ERRNO(-ret);					\
> +		ret = (typeof(ret))-1;					\
> +	}								\
> +	ret;								\
> +})
> +
> +#define __syscall(name, ...)						\
> +({									\
> +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> +	__syscall_ret(ret);						\
> +})

Well, I personally don't find that it increases legibility, on the
opposite. At first when reading the series, I thought you had dropped
errno setting on return. I think the reason is that when reading that
last macro, it's not at all obvious that __syscall_ret() is actually
modifying this ret value *and* returning it as the macro's result.

If we'd want to go down that route, I suspect that something like this
would at least hint about what is being returned:

+#define __syscall(name, ...)						\
+({									\
+	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
+	ret = __syscall_ret(ret);					\
+})

But I'm interested in others' opinion on this, particularly Thomas and
Arnd who review a significant number of patches. For now I prefer not
to take it before we've settled on a choice.

Thanks,
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Thomas Weißschuh]

On 2023-06-04 14:59:13+0200, Willy Tarreau wrote:
> Hi Zhangjin,
> 
> On Sun, Jun 04, 2023 at 01:34:29PM +0800, Zhangjin Wu wrote:
> > most of the library routines share the same code model, let's add some
> > macros to simplify the coding and shrink the code lines too.
> > 
> > One added for syscall return, one added for syscall call, both of them
> > can get the typeof 'return value' automatically.
> > 
> > To get the return type of syscalls, __auto_type is better than typeof(),
> > but it is not supported by the old compilers (before 2013, see [1]), so,
> > use typeof() here.
> > 
> > [1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html
> > 
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/include/nolibc/sys.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index 1d6f33f58629..937a8578e3d4 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -28,6 +28,21 @@
> >  #include "errno.h"
> >  #include "types.h"
> >  
> > +/* Syscall call and return helpers */
> > +#define __syscall_ret(ret)						\
> > +({									\
> > +	if (ret < 0) {							\
> > +		SET_ERRNO(-ret);					\
> > +		ret = (typeof(ret))-1;					\
> > +	}								\
> > +	ret;								\
> > +})
> > +
> > +#define __syscall(name, ...)						\
> > +({									\
> > +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> > +	__syscall_ret(ret);						\
> > +})
> 
> Well, I personally don't find that it increases legibility, on the
> opposite. At first when reading the series, I thought you had dropped
> errno setting on return. I think the reason is that when reading that
> last macro, it's not at all obvious that __syscall_ret() is actually
> modifying this ret value *and* returning it as the macro's result.
> 
> If we'd want to go down that route, I suspect that something like this
> would at least hint about what is being returned:
> 
> +#define __syscall(name, ...)						\
> +({									\
> +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> +	ret = __syscall_ret(ret);					\
> +})
> 
> But I'm interested in others' opinion on this, particularly Thomas and
> Arnd who review a significant number of patches. For now I prefer not
> to take it before we've settled on a choice.

While I see the value in factoring out this pattern I'm also not really
happy with the implementation.
Especially the magic delegation to "sys_##name".

What about something like this:

static inline long __ret_as_errno(long ret) /* or some other name */
{
	if (ret < 0) {
		SET_ERRNO(-ret);
		ret = -1;
	}
	return ret;
}

This avoids another macro by using a normal function.

Syscall return values should always fit into long, at least
extrapolating from syscall(2) and the fact that they are returned in
registers.

It would be a bit more verbose:

int chdir(const char *path)
{
	return __ret_as_errno(sys_chdir(path));
}

But it's clear what's going on and also just one line.

Thomas

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Thomas Weißschuh]

On 2023-06-04 14:59:13+0200, Willy Tarreau wrote:
> Hi Zhangjin,
> 
> On Sun, Jun 04, 2023 at 01:34:29PM +0800, Zhangjin Wu wrote:
> > most of the library routines share the same code model, let's add some
> > macros to simplify the coding and shrink the code lines too.
> > 
> > One added for syscall return, one added for syscall call, both of them
> > can get the typeof 'return value' automatically.
> > 
> > To get the return type of syscalls, __auto_type is better than typeof(),
> > but it is not supported by the old compilers (before 2013, see [1]), so,
> > use typeof() here.
> > 
> > [1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html
> > 
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/include/nolibc/sys.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index 1d6f33f58629..937a8578e3d4 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -28,6 +28,21 @@
> >  #include "errno.h"
> >  #include "types.h"
> >  
> > +/* Syscall call and return helpers */
> > +#define __syscall_ret(ret)						\
> > +({									\
> > +	if (ret < 0) {							\
> > +		SET_ERRNO(-ret);					\
> > +		ret = (typeof(ret))-1;					\
> > +	}								\
> > +	ret;								\
> > +})
> > +
> > +#define __syscall(name, ...)						\
> > +({									\
> > +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> > +	__syscall_ret(ret);						\
> > +})
> 
> Well, I personally don't find that it increases legibility, on the
> opposite. At first when reading the series, I thought you had dropped
> errno setting on return. I think the reason is that when reading that
> last macro, it's not at all obvious that __syscall_ret() is actually
> modifying this ret value *and* returning it as the macro's result.
> 
> If we'd want to go down that route, I suspect that something like this
> would at least hint about what is being returned:
> 
> +#define __syscall(name, ...)						\
> +({									\
> +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> +	ret = __syscall_ret(ret);					\
> +})
> 
> But I'm interested in others' opinion on this, particularly Thomas and
> Arnd who review a significant number of patches. For now I prefer not
> to take it before we've settled on a choice.

While I see the value in factoring out this pattern I'm also not really
happy with the implementation.
Especially the magic delegation to "sys_##name".

What about something like this:

static inline long __ret_as_errno(long ret) /* or some other name */
{
	if (ret < 0) {
		SET_ERRNO(-ret);
		ret = -1;
	}
	return ret;
}

This avoids another macro by using a normal function.

Syscall return values should always fit into long, at least
extrapolating from syscall(2) and the fact that they are returned in
registers.

It would be a bit more verbose:

int chdir(const char *path)
{
	return __ret_as_errno(sys_chdir(path));
}

But it's clear what's going on and also just one line.

Thomas

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Zhangjin Wu]

> On 2023-06-04 14:59:13+0200, Willy Tarreau wrote:
> > Hi Zhangjin,
> > 
> > On Sun, Jun 04, 2023 at 01:34:29PM +0800, Zhangjin Wu wrote:
> > > most of the library routines share the same code model, let's add some
> > > macros to simplify the coding and shrink the code lines too.
> > > 
> > > One added for syscall return, one added for syscall call, both of them
> > > can get the typeof 'return value' automatically.
> > > 
> > > To get the return type of syscalls, __auto_type is better than typeof(),
> > > but it is not supported by the old compilers (before 2013, see [1]), so,
> > > use typeof() here.
> > > 
> > > [1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html
> > > 
> > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > ---
> > >  tools/include/nolibc/sys.h | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > > index 1d6f33f58629..937a8578e3d4 100644
> > > --- a/tools/include/nolibc/sys.h
> > > +++ b/tools/include/nolibc/sys.h
> > > @@ -28,6 +28,21 @@
> > >  #include "errno.h"
> > >  #include "types.h"
> > >  
> > > +/* Syscall call and return helpers */
> > > +#define __syscall_ret(ret)						\
> > > +({									\
> > > +	if (ret < 0) {							\
> > > +		SET_ERRNO(-ret);					\
> > > +		ret = (typeof(ret))-1;					\
> > > +	}								\
> > > +	ret;								\
> > > +})
> > > +
> > > +#define __syscall(name, ...)						\
> > > +({									\
> > > +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> > > +	__syscall_ret(ret);						\
> > > +})
> > 
> > Well, I personally don't find that it increases legibility, on the
> > opposite. At first when reading the series, I thought you had dropped
> > errno setting on return. I think the reason is that when reading that
> > last macro,

Hi, Willy, I did add something like this in my local copy to pass the
errno and retval arguments too:

    #define __syscall_ret3(ret, errno, retval)				\
    ({									\
    	if (ret < 0) {							\
    		SET_ERRNO(errno);					\
    		ret = (typeof(ret)retval;				\
    	}								\
    	ret;								\
    })

    #define __syscall_ret(ret) __syscall_ret3(ret, -ret, -1)

But when really using them, I found we could be able to set the ret as errno at
first (and the reval is always -1 currently), then used the only simpler
__syscall_ret() at last.

> > it's not at all obvious that __syscall_ret() is actually
> > modifying this ret value *and* returning it as the macro's result.
> > 
> > If we'd want to go down that route, I suspect that something like this
> > would at least hint about what is being returned:
> > 
> > +#define __syscall(name, ...)						\
> > +({									\
> > +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> > +	ret = __syscall_ret(ret);					\
> > +})
> >

It is clearer.

> > But I'm interested in others' opinion on this, particularly Thomas and
> > Arnd who review a significant number of patches. For now I prefer not
> > to take it before we've settled on a choice.
> 
> While I see the value in factoring out this pattern I'm also not really
> happy with the implementation.
> Especially the magic delegation to "sys_##name".
> 
> What about something like this:
> 
> static inline long __ret_as_errno(long ret) /* or some other name */
> {
> 	if (ret < 0) {
> 		SET_ERRNO(-ret);
> 		ret = -1;
> 	}
> 	return ret;
> }
> 
> This avoids another macro by using a normal function.
>

It is reasonable, like it very much.

> Syscall return values should always fit into long, at least
> extra polating from syscall(2) and the fact that they are returned in
> registers.

Yes, I did use 'long' instead of 'int' for unistd.h locally, but since tried to
let it work with 'void *' before (e.g. sys_brk, an older version support pass
the errno value), so, the typeof() is used and the same to unistd.h, but at
last, none of (void *) return type is really used in current patchset, so, we
are able to use this normal function version without the checking of the type.

> 
> It would be a bit more verbose:
> 
> int chdir(const char *path)
> {
> 	return __ret_as_errno(sys_chdir(path));
> }
>
> But it's clear what's going on and also just one line.

Thanks Thomas, It looks good and I do like the 'embedded' calling of
sys_chrdir(path), but __syscall() looks cleaner and shorter too, let's put them
together:

int chdir(const char *path)
{
	return __ret_as_errno(sys_chdir(path));
}

int chdir(const char *path)
{
	return __syscall(chdir, path);
}

And even with:

int chdir(const char *path)
{
	return __sysret(sys_chdir(path));
}

__syscall() works likes syscall(), and the definition is similar to syscall(),
but uses the syscall name instead of syscall number, If reserve __syscall(),
the inline function would be renamed back to __syscall_ret() or something like
the shorter __sysret(), to align with our new __syscall(). 

for sys.h:

    /* Syscall return helper, set errno as ret when ret < 0 */
    static inline long __sysret(long ret)
    {
    	if (ret < 0) {
    		SET_ERRNO(-ret);
    		ret = -1;
    	}
    	return ret;
    }

    /* Syscall call helper, use syscall name instead of syscall number */
    #define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))

for unistd.h:

    #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))

What about this version?

The potential 'issue' may be mixing the use of __syscall(), _syscall() and
syscall(), but the compilers may help to fix up this for us, I don't think it
is a bottleneck.

Best regards,
Zhangjin

> 
> Thomas

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Zhangjin Wu]

> On 2023-06-04 14:59:13+0200, Willy Tarreau wrote:
> > Hi Zhangjin,
> > 
> > On Sun, Jun 04, 2023 at 01:34:29PM +0800, Zhangjin Wu wrote:
> > > most of the library routines share the same code model, let's add some
> > > macros to simplify the coding and shrink the code lines too.
> > > 
> > > One added for syscall return, one added for syscall call, both of them
> > > can get the typeof 'return value' automatically.
> > > 
> > > To get the return type of syscalls, __auto_type is better than typeof(),
> > > but it is not supported by the old compilers (before 2013, see [1]), so,
> > > use typeof() here.
> > > 
> > > [1]: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-11/msg01378.html
> > > 
> > > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > > ---
> > >  tools/include/nolibc/sys.h | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > > index 1d6f33f58629..937a8578e3d4 100644
> > > --- a/tools/include/nolibc/sys.h
> > > +++ b/tools/include/nolibc/sys.h
> > > @@ -28,6 +28,21 @@
> > >  #include "errno.h"
> > >  #include "types.h"
> > >  
> > > +/* Syscall call and return helpers */
> > > +#define __syscall_ret(ret)						\
> > > +({									\
> > > +	if (ret < 0) {							\
> > > +		SET_ERRNO(-ret);					\
> > > +		ret = (typeof(ret))-1;					\
> > > +	}								\
> > > +	ret;								\
> > > +})
> > > +
> > > +#define __syscall(name, ...)						\
> > > +({									\
> > > +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> > > +	__syscall_ret(ret);						\
> > > +})
> > 
> > Well, I personally don't find that it increases legibility, on the
> > opposite. At first when reading the series, I thought you had dropped
> > errno setting on return. I think the reason is that when reading that
> > last macro,

Hi, Willy, I did add something like this in my local copy to pass the
errno and retval arguments too:

    #define __syscall_ret3(ret, errno, retval)				\
    ({									\
    	if (ret < 0) {							\
    		SET_ERRNO(errno);					\
    		ret = (typeof(ret)retval;				\
    	}								\
    	ret;								\
    })

    #define __syscall_ret(ret) __syscall_ret3(ret, -ret, -1)

But when really using them, I found we could be able to set the ret as errno at
first (and the reval is always -1 currently), then used the only simpler
__syscall_ret() at last.

> > it's not at all obvious that __syscall_ret() is actually
> > modifying this ret value *and* returning it as the macro's result.
> > 
> > If we'd want to go down that route, I suspect that something like this
> > would at least hint about what is being returned:
> > 
> > +#define __syscall(name, ...)						\
> > +({									\
> > +	typeof(sys_##name(__VA_ARGS__)) ret = sys_##name(__VA_ARGS__);	\
> > +	ret = __syscall_ret(ret);					\
> > +})
> >

It is clearer.

> > But I'm interested in others' opinion on this, particularly Thomas and
> > Arnd who review a significant number of patches. For now I prefer not
> > to take it before we've settled on a choice.
> 
> While I see the value in factoring out this pattern I'm also not really
> happy with the implementation.
> Especially the magic delegation to "sys_##name".
> 
> What about something like this:
> 
> static inline long __ret_as_errno(long ret) /* or some other name */
> {
> 	if (ret < 0) {
> 		SET_ERRNO(-ret);
> 		ret = -1;
> 	}
> 	return ret;
> }
> 
> This avoids another macro by using a normal function.
>

It is reasonable, like it very much.

> Syscall return values should always fit into long, at least
> extra polating from syscall(2) and the fact that they are returned in
> registers.

Yes, I did use 'long' instead of 'int' for unistd.h locally, but since tried to
let it work with 'void *' before (e.g. sys_brk, an older version support pass
the errno value), so, the typeof() is used and the same to unistd.h, but at
last, none of (void *) return type is really used in current patchset, so, we
are able to use this normal function version without the checking of the type.

> 
> It would be a bit more verbose:
> 
> int chdir(const char *path)
> {
> 	return __ret_as_errno(sys_chdir(path));
> }
>
> But it's clear what's going on and also just one line.

Thanks Thomas, It looks good and I do like the 'embedded' calling of
sys_chrdir(path), but __syscall() looks cleaner and shorter too, let's put them
together:

int chdir(const char *path)
{
	return __ret_as_errno(sys_chdir(path));
}

int chdir(const char *path)
{
	return __syscall(chdir, path);
}

And even with:

int chdir(const char *path)
{
	return __sysret(sys_chdir(path));
}

__syscall() works likes syscall(), and the definition is similar to syscall(),
but uses the syscall name instead of syscall number, If reserve __syscall(),
the inline function would be renamed back to __syscall_ret() or something like
the shorter __sysret(), to align with our new __syscall(). 

for sys.h:

    /* Syscall return helper, set errno as ret when ret < 0 */
    static inline long __sysret(long ret)
    {
    	if (ret < 0) {
    		SET_ERRNO(-ret);
    		ret = -1;
    	}
    	return ret;
    }

    /* Syscall call helper, use syscall name instead of syscall number */
    #define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))

for unistd.h:

    #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))

What about this version?

The potential 'issue' may be mixing the use of __syscall(), _syscall() and
syscall(), but the compilers may help to fix up this for us, I don't think it
is a bottleneck.

Best regards,
Zhangjin

> 
> Thomas

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Willy Tarreau]

On Mon, Jun 05, 2023 at 01:58:57PM +0800, Zhangjin Wu wrote:
> > What about something like this:
> > 
> > static inline long __ret_as_errno(long ret) /* or some other name */
> > {
> > 	if (ret < 0) {
> > 		SET_ERRNO(-ret);
> > 		ret = -1;
> > 	}
> > 	return ret;
> > }
> > 
> > This avoids another macro by using a normal function.
> >
> 
> It is reasonable, like it very much.
> 
> > Syscall return values should always fit into long, at least
> > extra polating from syscall(2) and the fact that they are returned in
> > registers.
> 
> Yes, I did use 'long' instead of 'int' for unistd.h locally, but since tried to
> let it work with 'void *' before (e.g. sys_brk, an older version support pass
> the errno value), so, the typeof() is used and the same to unistd.h, but at
> last, none of (void *) return type is really used in current patchset, so, we
> are able to use this normal function version without the checking of the type.
> 
> > 
> > It would be a bit more verbose:
> > 
> > int chdir(const char *path)
> > {
> > 	return __ret_as_errno(sys_chdir(path));
> > }
> >
> > But it's clear what's going on and also just one line.
> 
> Thanks Thomas, It looks good and I do like the 'embedded' calling of
> sys_chrdir(path), but __syscall() looks cleaner and shorter too, let's put them
> together:
> 
> int chdir(const char *path)
> {
> 	return __ret_as_errno(sys_chdir(path));
> }
> 
> int chdir(const char *path)
> {
> 	return __syscall(chdir, path);
> }
> 
> And even with:
> 
> int chdir(const char *path)
> {
> 	return __sysret(sys_chdir(path));
> }
> 
> __syscall() works likes syscall(), and the definition is similar to syscall(),
> but uses the syscall name instead of syscall number, If reserve __syscall(),
> the inline function would be renamed back to __syscall_ret() or something like
> the shorter __sysret(), to align with our new __syscall(). 
> 
> for sys.h:
> 
>     /* Syscall return helper, set errno as ret when ret < 0 */
>     static inline long __sysret(long ret)
>     {
>     	if (ret < 0) {
>     		SET_ERRNO(-ret);
>     		ret = -1;
>     	}
>     	return ret;
>     }
> 
>     /* Syscall call helper, use syscall name instead of syscall number */
>     #define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))
> 
> for unistd.h:
> 
>     #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))
> 
> What about this version?
> 
> The potential 'issue' may be mixing the use of __syscall(), _syscall() and
> syscall(), but the compilers may help to fix up this for us, I don't think it
> is a bottleneck.

I think that could work. However, please place __attribute__((always_inline))
on these inline functions, as we don't want to turn them to function calls
even at -O0.

I'm traveling today, I'll let you and Thomas debate and decide how you'd
like this to evolve.

Also, please note that Paul is OK with merging for 6.5, but we should
absolutely limit last-minute changes to the strict minimum we're able
to test now.

Thanks!
Willy

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Willy Tarreau]

On Mon, Jun 05, 2023 at 01:58:57PM +0800, Zhangjin Wu wrote:
> > What about something like this:
> > 
> > static inline long __ret_as_errno(long ret) /* or some other name */
> > {
> > 	if (ret < 0) {
> > 		SET_ERRNO(-ret);
> > 		ret = -1;
> > 	}
> > 	return ret;
> > }
> > 
> > This avoids another macro by using a normal function.
> >
> 
> It is reasonable, like it very much.
> 
> > Syscall return values should always fit into long, at least
> > extra polating from syscall(2) and the fact that they are returned in
> > registers.
> 
> Yes, I did use 'long' instead of 'int' for unistd.h locally, but since tried to
> let it work with 'void *' before (e.g. sys_brk, an older version support pass
> the errno value), so, the typeof() is used and the same to unistd.h, but at
> last, none of (void *) return type is really used in current patchset, so, we
> are able to use this normal function version without the checking of the type.
> 
> > 
> > It would be a bit more verbose:
> > 
> > int chdir(const char *path)
> > {
> > 	return __ret_as_errno(sys_chdir(path));
> > }
> >
> > But it's clear what's going on and also just one line.
> 
> Thanks Thomas, It looks good and I do like the 'embedded' calling of
> sys_chrdir(path), but __syscall() looks cleaner and shorter too, let's put them
> together:
> 
> int chdir(const char *path)
> {
> 	return __ret_as_errno(sys_chdir(path));
> }
> 
> int chdir(const char *path)
> {
> 	return __syscall(chdir, path);
> }
> 
> And even with:
> 
> int chdir(const char *path)
> {
> 	return __sysret(sys_chdir(path));
> }
> 
> __syscall() works likes syscall(), and the definition is similar to syscall(),
> but uses the syscall name instead of syscall number, If reserve __syscall(),
> the inline function would be renamed back to __syscall_ret() or something like
> the shorter __sysret(), to align with our new __syscall(). 
> 
> for sys.h:
> 
>     /* Syscall return helper, set errno as ret when ret < 0 */
>     static inline long __sysret(long ret)
>     {
>     	if (ret < 0) {
>     		SET_ERRNO(-ret);
>     		ret = -1;
>     	}
>     	return ret;
>     }
> 
>     /* Syscall call helper, use syscall name instead of syscall number */
>     #define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))
> 
> for unistd.h:
> 
>     #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))
> 
> What about this version?
> 
> The potential 'issue' may be mixing the use of __syscall(), _syscall() and
> syscall(), but the compilers may help to fix up this for us, I don't think it
> is a bottleneck.

I think that could work. However, please place __attribute__((always_inline))
on these inline functions, as we don't want to turn them to function calls
even at -O0.

I'm traveling today, I'll let you and Thomas debate and decide how you'd
like this to evolve.

Also, please note that Paul is OK with merging for 6.5, but we should
absolutely limit last-minute changes to the strict minimum we're able
to test now.

Thanks!
Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Zhangjin Wu]

> On Mon, Jun 05, 2023 at 01:58:57PM +0800, Zhangjin Wu wrote:
> > > What about something like this:
> > > 
> > > static inline long __ret_as_errno(long ret) /* or some other name */
> > > {
> > > 	if (ret < 0) {
> > > 		SET_ERRNO(-ret);
> > > 		ret = -1;
> > > 	}
> > > 	return ret;
> > > }
> > > 
> > > This avoids another macro by using a normal function.
> > >
> > 
> > It is reasonable, like it very much.
> > 
> > > Syscall return values should always fit into long, at least
> > > extra polating from syscall(2) and the fact that they are returned in
> > > registers.
> > 
> > Yes, I did use 'long' instead of 'int' for unistd.h locally, but since tried to
> > let it work with 'void *' before (e.g. sys_brk, an older version support pass
> > the errno value), so, the typeof() is used and the same to unistd.h, but at
> > last, none of (void *) return type is really used in current patchset, so, we
> > are able to use this normal function version without the checking of the type.
> > 
> > > 
> > > It would be a bit more verbose:
> > > 
> > > int chdir(const char *path)
> > > {
> > > 	return __ret_as_errno(sys_chdir(path));
> > > }
> > >
> > > But it's clear what's going on and also just one line.
> > 
> > Thanks Thomas, It looks good and I do like the 'embedded' calling of
> > sys_chrdir(path), but __syscall() looks cleaner and shorter too, let's put them
> > together:
> > 
> > int chdir(const char *path)
> > {
> > 	return __ret_as_errno(sys_chdir(path));
> > }
> > 
> > int chdir(const char *path)
> > {
> > 	return __syscall(chdir, path);
> > }
> > 
> > And even with:
> > 
> > int chdir(const char *path)
> > {
> > 	return __sysret(sys_chdir(path));
> > }
> > 
> > __syscall() works likes syscall(), and the definition is similar to syscall(),
> > but uses the syscall name instead of syscall number, If reserve __syscall(),
> > the inline function would be renamed back to __syscall_ret() or something like
> > the shorter __sysret(), to align with our new __syscall(). 
> > 
> > for sys.h:
> > 
> >     /* Syscall return helper, set errno as ret when ret < 0 */
> >     static inline long __sysret(long ret)
> >     {
> >     	if (ret < 0) {
> >     		SET_ERRNO(-ret);
> >     		ret = -1;
> >     	}
> >     	return ret;
> >     }
> > 
> >     /* Syscall call helper, use syscall name instead of syscall number */
> >     #define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))
> > 
> > for unistd.h:
> > 
> >     #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))
> > 
> > What about this version?
> > 
> > The potential 'issue' may be mixing the use of __syscall(), _syscall() and
> > syscall(), but the compilers may help to fix up this for us, I don't think it
> > is a bottleneck.
> 
> I think that could work. However, please place __attribute__((always_inline))
> on these inline functions, as we don't want to turn them to function calls
> even at -O0.

Thanks, done.

> 
> I'm traveling today, I'll let you and Thomas debate and decide how you'd
> like this to evolve.
> 

Happy traveling.

This revision is basically derived from the 'long' type information and
__ret_as_errno() from Thomas, I will wait suggestion from Thomas and then send
v2 later.

> Also, please note that Paul is OK with merging for 6.5, but we should
> absolutely limit last-minute changes to the strict minimum we're able
> to test now.

Strongly agree, we can delay this and the left time64 syscalls to 6.6, because
they require more cleanup and discussion.

Best regards,
Zhangjin

> 
> Thanks!
> Willy

Re: [PATCH 1/4] tools/nolibc: unistd.h: add __syscall() and __syscall_ret() helpers

[Zhangjin Wu]

> On Mon, Jun 05, 2023 at 01:58:57PM +0800, Zhangjin Wu wrote:
> > > What about something like this:
> > > 
> > > static inline long __ret_as_errno(long ret) /* or some other name */
> > > {
> > > 	if (ret < 0) {
> > > 		SET_ERRNO(-ret);
> > > 		ret = -1;
> > > 	}
> > > 	return ret;
> > > }
> > > 
> > > This avoids another macro by using a normal function.
> > >
> > 
> > It is reasonable, like it very much.
> > 
> > > Syscall return values should always fit into long, at least
> > > extra polating from syscall(2) and the fact that they are returned in
> > > registers.
> > 
> > Yes, I did use 'long' instead of 'int' for unistd.h locally, but since tried to
> > let it work with 'void *' before (e.g. sys_brk, an older version support pass
> > the errno value), so, the typeof() is used and the same to unistd.h, but at
> > last, none of (void *) return type is really used in current patchset, so, we
> > are able to use this normal function version without the checking of the type.
> > 
> > > 
> > > It would be a bit more verbose:
> > > 
> > > int chdir(const char *path)
> > > {
> > > 	return __ret_as_errno(sys_chdir(path));
> > > }
> > >
> > > But it's clear what's going on and also just one line.
> > 
> > Thanks Thomas, It looks good and I do like the 'embedded' calling of
> > sys_chrdir(path), but __syscall() looks cleaner and shorter too, let's put them
> > together:
> > 
> > int chdir(const char *path)
> > {
> > 	return __ret_as_errno(sys_chdir(path));
> > }
> > 
> > int chdir(const char *path)
> > {
> > 	return __syscall(chdir, path);
> > }
> > 
> > And even with:
> > 
> > int chdir(const char *path)
> > {
> > 	return __sysret(sys_chdir(path));
> > }
> > 
> > __syscall() works likes syscall(), and the definition is similar to syscall(),
> > but uses the syscall name instead of syscall number, If reserve __syscall(),
> > the inline function would be renamed back to __syscall_ret() or something like
> > the shorter __sysret(), to align with our new __syscall(). 
> > 
> > for sys.h:
> > 
> >     /* Syscall return helper, set errno as ret when ret < 0 */
> >     static inline long __sysret(long ret)
> >     {
> >     	if (ret < 0) {
> >     		SET_ERRNO(-ret);
> >     		ret = -1;
> >     	}
> >     	return ret;
> >     }
> > 
> >     /* Syscall call helper, use syscall name instead of syscall number */
> >     #define __syscall(name, ...) __sysret(sys_##name(__VA_ARGS__))
> > 
> > for unistd.h:
> > 
> >     #define _syscall(N, ...) __sysret(my_syscall##N(__VA_ARGS__))
> > 
> > What about this version?
> > 
> > The potential 'issue' may be mixing the use of __syscall(), _syscall() and
> > syscall(), but the compilers may help to fix up this for us, I don't think it
> > is a bottleneck.
> 
> I think that could work. However, please place __attribute__((always_inline))
> on these inline functions, as we don't want to turn them to function calls
> even at -O0.

Thanks, done.

> 
> I'm traveling today, I'll let you and Thomas debate and decide how you'd
> like this to evolve.
> 

Happy traveling.

This revision is basically derived from the 'long' type information and
__ret_as_errno() from Thomas, I will wait suggestion from Thomas and then send
v2 later.

> Also, please note that Paul is OK with merging for 6.5, but we should
> absolutely limit last-minute changes to the strict minimum we're able
> to test now.

Strongly agree, we can delay this and the left time64 syscalls to 6.6, because
they require more cleanup and discussion.

Best regards,
Zhangjin

> 
> Thanks!
> Willy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv