Hi, Willy
> On Sat, Aug 12, 2023 at 05:51:53AM +0800, Zhangjin Wu wrote:
> > As reported and suggested by Willy, the inline __sysret() helper
> > introduces three types of conversions and increases the size:
> >
> > (1) the "unsigned long" argument to __sysret() forces a sign extension
> > from all sys_* functions that used to return 'int'
> >
> > (2) the comparison with the error range now has to be performed on a
> > 'unsigned long' instead of an 'int'
> >
> > (3) the return value from __sysret() is a 'long' (note, a signed long)
> > which then has to be turned back to an 'int' before being returned by the
> > caller to satisfy the caller's prototype.
> >
> > To fix up this, firstly, let's use macro instead of inline function to
> > preserves the input type and avoids these useless conversions (1), (3).
> >
> > Secondly, since all of the sys_* functions have been converted to return
> > integer, now, it is able to remove comparison to a 'unsigned long'
> > -MAX_ERRNO (2) and restore the simple sign comparison as before.
> >
> (...)
> > +/* Syscall return helper, set errno as -ret when ret < 0 */
> > +#define __sysret(arg) \
> > +({ \
> > + __typeof__(arg) __ret = (arg); \
> > + if (__ret < 0) { \
> > + SET_ERRNO(-__ret); \
> > + __ret = -1L; \
> > + } \
> > + __ret; \
> > +})
>
> Except that this now breaks brk(), mmap() and sbrk() by taking any value
> with MSB set as an error. Also you've re-introduced the problem you've
> faced with const. See my simplification in the other thread by using "?:"
> which does avoids any assignment.
>
Yeah, thanks for your explanation in this reply [1], the 'const' flag
only triggers build error on the second 'assign' (__ret == -1L), the
first 'assign' is a definition, it is not problematic. so, your "?:"
method is a great idea to simply return without the second 'assign'.
> Let's just roll brk(), mmap() and sbrk() to their original, working,
> definition:
>
> static __attribute__((unused))
> void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset)
> {
> void *ret = sys_mmap(addr, length, prot, flags, fd, offset);
>
> if ((unsigned long)ret >= -MAX_ERRNO) {
> SET_ERRNO(-(long)ret);
> ret = MAP_FAILED;
> }
> return ret;
> }
>
Agree, only left a suggestion here [2] about whether we can apply the 2nd patch
instead of rolling them back, let's discuss it in [2] thread.
> And we're done, you can then keep the simplified __sysret() macro for all
> other call places.
>
Now, this issue is near to the end ;-)
Thanks!
Zhangjin
---
[1]: https://lore.kernel.org/lkml/20230813085140.GD8237@xxxxxx/#R
[2]: https://lore.kernel.org/lkml/20230813132620.19411-1-falcon@xxxxxxxxxxx/
> Willy
