| Message ID | 96624cc918092737d35dd539d184de06dba7a9b8.1691788036.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: fix up size inflat regression | expand |
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. 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; } And we're done, you can then keep the simplified __sysret() macro for all other call places. Willy
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
On Sun, Aug 13, 2023 at 09:39:44PM +0800, Zhangjin Wu wrote: > > 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. (...) > [2]: https://lore.kernel.org/lkml/20230813132620.19411-1-falcon@tinylab.org/ I'm sorry but I can't find this "suggestion" in this yet-another-super- long-description-of-another-idea-of-redesign. In addition it's extremely painful to constantly have to go through web links to follow a single conversation. Mail works in threads for a reason. When the same discussion is handled in many parallel threads it becomes impossible to keep it focused on a specific topic. This is also why you should stop systematically responding to a message with yet another redesign suggestion, this is super hard to follow and it literally takes me several hours a week! And at the end we've not addressed the initial problem but discussed plenty of other things. Thanks, Willy
On Sun, Aug 13, 2023 at 09:39:44PM +0800, Zhangjin Wu wrote: > > 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 ;-) I've now pushed the simplified fix (without changing the SET_ERRNO() macro, enough last minute breaking changes for now) in branch 20230815-for-6.6-2. The tests pass and riscv/loongarch are even very slightly smaller than before (~8 bytes) but again that doesn't count as it depends on how the compiler decides to arrange if/else branches. I'll let Shuah know about these late fixes. Regards, Willy
> On Sun, Aug 13, 2023 at 09:39:44PM +0800, Zhangjin Wu wrote: > > > 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 ;-) > > I've now pushed the simplified fix (without changing the SET_ERRNO() > macro, enough last minute breaking changes for now) in branch > 20230815-for-6.6-2. > > The tests pass and riscv/loongarch are even very slightly smaller than > before (~8 bytes) but again that doesn't count as it depends on how the > compiler decides to arrange if/else branches. > Tested 20230815-for-6.6-2 with latest Arnd's gcc 13.2.0 (left: old, right: new), no warning, no failure: // run-user $ for arch in ${ARCHS[@]}; do printf "%9s: " $arch; make run-user XARCH=$arch | grep status | tr '\n' ' '; \ size nolibc-test | tail -1 | tr '\t' ' ' | tr -s ' ' | cut -d ' ' -f2; done i386: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 19654 > 19508 x86_64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22337 > 22011 arm64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26292 > 25868 arm: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 23140 > 23112 mips: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 23164 > 22924 // mips-linux- has smaller size, here uses mips64 ppc: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 26812 > 26628 ppc64: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 27380 > 27204 ppc64le: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 28004 > 27828 riscv: 160 test(s): 158 passed, 2 skipped, 0 failed => status: warning 22062 > 21794 s390: 160 test(s): 157 passed, 3 skipped, 0 failed => status: warning 22592 > 22192 // kernel build + run arch/board | result ------------|------------ arm/vexpress-a9 | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. arm/virt | 160 test(s): 156 passed, 4 skipped, 0 failed => status: warning. aarch64/virt | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. ppc/g3beige | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. ppc/ppce500 | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. ppc64le/pseries | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. ppc64le/powernv | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. ppc64/pseries | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. ppc64/powernv | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. i386/pc | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. x86_64/pc | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. mipsel/malta | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. loongarch64/virt | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. riscv64/virt | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. s390x/s390-ccw-virtio | 160 test(s): 159 passed, 1 skipped, 0 failed => status: warning. Thanks, Zhangjin > I'll let Shuah know about these late fixes. > > Regards, > Willy
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index a28e7fbff448..e0b68d3532b6 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -27,23 +27,16 @@ #include "errno.h" #include "types.h" - -/* Syscall return helper for library routines, set errno as -ret when ret is in - * range of [-MAX_ERRNO, -1] - * - * Note, No official reference states the errno range here aligns with musl - * (src/internal/syscall_ret.c) and glibc (sysdeps/unix/sysv/linux/sysdep.h) - */ - -static __inline__ __attribute__((unused, always_inline)) -long __sysret(unsigned long ret) -{ - if (ret >= (unsigned long)-MAX_ERRNO) { - SET_ERRNO(-(long)ret); - return -1; - } - return ret; -} +/* 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; \ +}) /* Functions in this file only describe syscalls. They're declared static so * that the compiler usually decides to inline them while still being allowed
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. Suggested-by: Willy Tarreau <w@1wt.eu> Link: https://lore.kernel.org/lkml/20230806095846.GB10627@1wt.eu/ Signed-off-by: Zhangjin Wu <falcon@tinylab.org> --- tools/include/nolibc/sys.h | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)