| Message ID | f549b27981484b429b7c7f98e212bf3c5561724f.1685856497.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/nolibc: add two new syscall helpers | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
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
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
> 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
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
> 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
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
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(+)