Re: [PATCH v6 2/2] tools/nolibc: fix up size inflate regression

[Zhangjin Wu]

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