Messages in this thread | | | From | Zhangjin Wu <> | Subject | Re: [PATCH v6 2/2] tools/nolibc: fix up size inflate regression | Date | Sun, 13 Aug 2023 21:39:44 +0800 |
| |
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@1wt.eu/#R [2]: https://lore.kernel.org/lkml/20230813132620.19411-1-falcon@tinylab.org/
> Willy
| |