Hi, Willy
> On Sun, Aug 13, 2023 at 09:26:20PM +0800, Zhangjin Wu wrote:
> [...]
> > With this exception, s390 no long need to provide its own mmap
> > definition, it (seems i386 too, but it uses mmap2 currently) can simply
> > define '__ARCH_WANT_SYS_OLD_MMAP' as the '__ARCH_WANT_SYS_OLD_SELECT' we
> > are using for old_select.
> >
> > The same method applies to the selection of the different backward
> > version of the sys_clone() syscall (from kernel/fork.c):
> (...)
>
> > #ifdef __NR_clone
> > #undef sys_clone
> > #define __sys_clone(...) __sysdef(clone, __VA_ARGS__)
> >
> > static __attribute__((unused))
> > int sys_clone(unsigned long clone_flags, unsigned long newsp,
> > int __attribute__((unused)) stack_size,
> > int parent_tidptr, int child_tidptr, unsigned long tls)
> > {
> > long ret;
> > #ifdef __ARCH_WANT_SYS_CLONE_BACKWARDS
> > ret = __sys_clone(clone_flags, newsp, parent_tidptr, tls, child_tidptr);
> > #elif defined(__ARCH_WANT_SYS_CLONE_BACKWARDS2)
> > ret = __sys_clone(newsp, clone_flags, parent_tidptr, child_tidptr, tls);
> > #elif defined(__ARCH_WANT_SYS_CLONE_BACKWARDS3)
> > ret = __sys_clone(clone_flags, newsp, stack_size, parent_tidptr, child_tidptr, tls);
> > #else
> > ret = __sys_clone(clone_flags, newsp, parent_tidptr, child_tidptr, tls);
> > #endif
> > return ret;
> > }
> > #endif /* __NR_clone */
> >
> > s390 only requires to define '__ARCH_WANT_SYS_CLONE_BACKWARDS2', no need
> > to provide its own sys_fork() version, in the __NR_clone branch of
> > fork(), __ARCH_WANT_SYS_CLONE_BACKWARDS2 can directly select the right
> > version of sys_clone() for s390).
>
> Maybe but with much less #define indirections it would be significantly
> better.
>
> (...)
> > We only have these three exceptions currently, with this normalization,
> > the library routines from sys.h can directly think sys_* macros are
> > generic, if not, let syscall.h take care of the right exceptions.
>
> I see the point. But that doesn't remove the need to write the exported
> function itself. I'm not saying there's nothing to save here, I see your
> point, I'm just wondering if we really gain something in terms of ease
> of declaring new syscalls especially for first-time contributors and if
> we're not losing in maintenance. If at least it's easy enough to implement
> exceptions, maybe it could be worth further investigating.
>
I will delay the whole work about __sysdef(), but work on some more generic
parts (like the exceptions above) at first.
> > > > static __attribute__((unused))
> > > > int dup2(int old, int new)
> > > > {
> > > > int ret = sys_dup3(old, new, 0);
> > > >
> > > > if (ret == -ENOSYS)
> > > > ret = sys_dup2(old, new);
> > > >
> > > > return __sysret(ret);
> > > > }
> > >
> > > But this will add a useless test after all such syscalls, we'd rather
> > > not do that!
> > >
> >
> > Indeed, I found this issue too, when __NR_dup3 not defined, it returns
> > -ENOSYS, than, no size issue, otherwise, the compiler will not be able
> > to learn what the ret of sys_dup3() will be, so, it can not optimize the
> > second call to sys_dup2().
> >
> > So, the '#ifdef' logic must be used like we did in sys_* functions, but
> > it is really not that meaningful (no big gain as you mentioned above) if
> > we only move them from the sys_* functions to the library routines.
> >
> > At last, I found the ternary operation together with the initialization
> > of the not-defined __NR_* as NOLIBC__NR_NOSYS help this a lot, at last,
> > we get something like this:
> >
> > /* __systry2() is used to select one of two provided low level syscalls */
> > #define __systry2(a, sys_a, sys_b) \
> > ((NOLIBC__NR_##a != NOLIBC__NR_NOSYS) ? (sys_a) : (sys_b))
>
> But this supposes that all of them are manually defined as you did above.
> I'd rather implement an ugly is_numeric() macro based on argument
> resolution. I've done it once in another project, I don't remember
> precisely where it is but I vaguely remember that it used to check
> that the string resolution of the argument gave a letter (when it
> does not exist) or a digit (when it does). I can look into that later
> if needed. But please avoid extra macro definitions as much as possible,
> they're a real pain to handle in the code. There's no error when one is
> missing or has a typo, it's difficult to follow them and they don't
> appear in the debugger.
>
Yeah, your reply inspired me to look into the IS_ENABLED() from
../include/linux/kconfig.h macro again, there was a __is_defined() there, let's
throw away the ugly sysnr.h. I thought of IS_ENABLED() was only for y/n/m
before, but it does return 0 when the macro is not defined, it uses the same
trick in syscall() to calculate the number of arguments, if the macro is not
defined, then, 0 "argument".
> > It can eliminate all of the '#ifdef' stuffs, using the chmod example you
> > mentioned above, it becomes something like this:
> >
> > /*
> > * int chmod(const char *path, mode_t mode);
> > */
> >
> > static __attribute__((unused))
> > int chmod(const char *path, mode_t mode)
> > {
> > return __sysret(__systry2(chmod, sys_chmod(path, mode), sys_fchmodat(AT_FDCWD, path, mode, 0)));
> > }
> >
> > Purely clean and clear.
>
> That's a matter of taste and it may explain why we often disagree. For me
> it's horrid. If I'm the one implementing chmod for my platform and it does
> not work, what should I do when facing that, except want to cry ? Think
> that right now we have this:
>
> static __attribute__((unused))
> int sys_chmod(const char *path, mode_t mode)
> {
> #ifdef __NR_fchmodat
> return my_syscall4(__NR_fchmodat, AT_FDCWD, path, mode, 0);
> #elif defined(__NR_chmod)
> return my_syscall2(__NR_chmod, path, mode);
> #else
> return -ENOSYS;
> #endif
> }
>
> Sure it can be called not pretty, but I think it has the merit of being
> totally explicit, and whoever sees chmod() fail can quickly check based
> on the test in what situation they're supposed to be and what to check.
>
> One thing I'm worried about also regarding using my_syscall() without the
> number is that it's easy to miss an argument and have random values taken
> from registers (or the stack) passed as argument. For example above we can
> see that the flags part is 0 in fchmodat(). It's easy to miss themn and
> while the syscall4() will complain, syscall() will silently turn that
> into syscall3(). That's not necessarily a big deal, but we've seen during
> the development that it's easy to make mistakes and they're not trivial
> to spot. So again I'm really wondering about the benefits in such a case.
>
> This is well illustrated in your example below:
>
> > return __sysret(__systry2(newselect, sys_newselect(nfds, rfds, wfds, efds, timeout),
> > sys_pselect6(nfds, rfds, wfds, efds, timeout ? &t : NULL, NULL)));
>
> How many attempts to get it right ? Just skip one NULL and you don't
> see it.
Yeah, seems we have missed the last 0 in ppoll() before and the test may not
report about it either.
[...]
> > > Sure it's not pretty, and I'd rather just go back to SET_ERRNO() to be
> > > honest, because we're there just because of the temptation to remove
> > > lines that were not causing any difficulties :-/
> > >
> > > I think we can do something in-between and deal only with signed returns,
> > > and explicitly place the test for MAX_ERRNO on the two unsigned ones
> > > (brk and mmap). It should look approximately like this:
> > >
> > > #define __sysret(arg) \
> > > ({ \
> > > __typeof__(arg) __sysret_arg = (arg); \
> > > (__sysret_arg < 0) ? ({ /* error ? */ \
> > > SET_ERRNO(-__sysret_arg); /* yes: errno != -ret */ \
> > > ((__typeof__(arg)) -1); /* return -1 */ \
> > > }) : __sysret_arg; /* return original value */ \
> > > })
> > >
> >
> > I like this one very much, a simple test shows, it saves one more byte.
>
> I'm going to do that and revert the 3 affected syscalls.
>
Ok.
> > Only a quesiton, why 'errno != -ret' has a '!'? and we have post-tab in
> > above two lines of __sysret() too, I have changed them to whitespaces.
>
[...]
>
> > But let them align with the others may be better, so, most of the sys_*
> > macros can be simply mapped with a simple line (all of them are
> > generated automatically), without the care of the return types changing.
> >
> > So, Willy, as a summary:
> >
> > - one solution is your new __sysret() + restore the original SET_ERRNO
> > for mmap and brk [1].
> >
> > - another solution is your new __sysret() + my patch [2] to let mmap and brk
> > return 'long' as the other sys_* function does.
>
> No, because it will completely break them when they'll need to access the
> second half of the memory, as I already explained somewhere else in one
> of these numerous discussions.
>
Sorry, will recheck this part later, please ignore it.
[...]
>
> > I will resell my proposed patchset above, at
> > least a RFC patchset, please ignore this currently ;-)
>
> Please allow me to breathe a little bit. Really I mean, I'm already worn
> by having to constantly review breaking changes that either introduce
> bugs or break maintainability, and to have to justify myself for things
> that I thought would be obvious to anyone. Massive changes are extremely
> time consuming to review, and trying to figure subtle breakage in such
> low-level stuff is even harder. I simply can't assign more time to this,
> particularly for the expected gains which for me or often perceived as
> losses of maintainability instead :-/
>
Take a rest, I will delay the whole __sysdef() stuff and continue to work on
the last tinyconfig patchset after v6.5, it is the last one before our early
rv32 work ;-)
Thanks,
Zhangjin
> Thanks,
> Willy
