[PATCH v6 0/2] tools/nolibc: fix up size inflat regression

[PATCH v6 0/2] tools/nolibc: fix up size inflat regression

[Zhangjin Wu]

Hi, Willy

As we discussed in v5, I have proposed a my_syscall() macro, it can
convert all of the sys_* functions to macros, and such macros can simply
preserve input types from library routines and inherit the 'long' return
type from my_syscall<N>. As a result, our __sysret() helper will only
require to accept integer types and therefore we can simply revert it to
our old sign comparison version (but as macro).

I have already prepared a series of my_syscall() patchset but it
includes several not that simple patches, before sending it for review,
to directly solve the __sysret() issue at first, it is better to only
convert the current three sys_* functions to return 'long' instead of
pointer, which will make things easier.

Here is the testing result on all archs (except loongarch) with Arnd's
gcc 13.2.0, before testing it, we'd better apply the CROSS_COMPILE
patchset [1] manually:

    // before
    $ ARCHS="i386 x86_64 arm64 arm mips ppc ppc64 ppc64le riscv s390"
    $ for arch in ${ARCHS[@]}; do printf "%9s: " $arch; make run-user XARCH=$arch 2>/dev/null | 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
       x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22337
        arm64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26292
          arm: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 23140
         mips: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 23164
          ppc: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26812
        ppc64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27380
      ppc64le: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 28004
        riscv: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22062
         s390: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22592 

    // after
    $ for arch in ${ARCHS[@]}; do printf "%9s: " $arch; make run-user XARCH=$arch 2>/dev/null | 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 19502
       x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22000
        arm64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 25860
          arm: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 23108
         mips: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22908
          ppc: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26616
        ppc64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27192
      ppc64le: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27816
        riscv: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21790
         s390: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22184

    // compare
         i386: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 19654 -> 19502
       x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22337 -> 22000
        arm64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26292 -> 25860
          arm: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 23140 -> 23108
         mips: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 23164 -> 22908
          ppc: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26812 -> 26616
        ppc64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27380 -> 27192
      ppc64le: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 28004 -> 27816
        riscv: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22062 -> 21790
         s390: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22592 -> 22184

After these two patches, will send the proposed my_syscall() patchset
tomorrow, it can even further reduce more type conversions and therefore
reduce more binary bytes, here is a preview of the testing result:

   // with the coming my_syscall() patchset, sys_* from functionsn to macros
     i386: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 19250
   x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21733
    arm64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 25804
      arm: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22828
     mips: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22740
      ppc: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26376
    ppc64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26752
  ppc64le: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27360
    riscv: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21746
     s390: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 21928

   // compare: __sysret() function -> __sysret() macro -> sys_* macros
         i386: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 19654 -> 19502 -> 19250 
       x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22337 -> 22000 -> 21733
        arm64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26292 -> 25860 -> 25804
          arm: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 23140 -> 23108 -> 22828
         mips: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 23164 -> 22908 -> 22740
          ppc: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 26812 -> 26616 -> 26376
        ppc64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 27380 -> 27192 -> 26752
      ppc64le: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 28004 -> 27816 -> 27360
        riscv: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 22062 -> 21790 -> 21746
         s390: 160 test(s): 157 passed,   3 skipped,   0 failed => status: warning 22592 -> 22184 -> 21928

It can also shrink the whole sys.h from 1171 lines to around 738 lines.

Changes from v5 --> v6:

* The method introduced in v5 works but it is too complex ;-)

* Convert the return type of sys_brk/mmap/mmap2 from pointer to 'long'
  (like my_syscall<N> does), after this, all of the sys_* functions
  return integer.

* Restore __sysret() helper to sign comparison as originally, but also
  use macro instead of inline function to avoid useless input type and
  return type conversion.

Changes from v4 --> v5:

* Use __typeof__((arg) + 0) to lose the 'const' flag for old gcc
  versions.

* Import the famous __is_constexpr() macro from kernel side and add a
  __is_pointer() macro based on it. (David, to avoid introduce extra
  discuss on the prove-in-use __is_constexpr macro, this patch uses the
  original version instead of your suggested version, more info here:
  https://lore.kernel.org/lkml/20220131204357.1133674-1-keescook@chromium.org/)

* Use __builtin_choose_expr() to merge two comparisons to share the same
  errno setting code and the -1L assignment code.

Changes from v3 --> v4:

* fix up a new warning about 'ret < 0' when the input arg type is (void *)

Changes from v2 --> v3:

* define a __GXX_HAS_AUTO_TYPE_WITH_CONST_SUPPORT for gcc >= 11.0 (ABI_VERSION >= 1016)
* split __sysret() to two versions by the macro instead of a mixed unified and unreadable version
* use shorter __ret instead of __sysret_arg

Changes from v1 --> v2:

* fix up argument with 'const' in the type
* support "void *" argument


Best regards,
Zhangjin Wu
---

v5: https://lore.kernel.org/lkml/b6ff2684f557f6ce00151905990643e651391614.1691437328.git.falcon@tinylab.org/
v4: https://lore.kernel.org/lkml/a4084f7fac7a89f861b5582774bc7a98634d1e76.1691392805.git.falcon@tinylab.org/
v3: https://lore.kernel.org/lkml/8eaab5da2dcbba42e3f3efc2ae686a22c95f84f0.1691386601.git.falcon@tinylab.org/
v2: https://lore.kernel.org/lkml/95fe3e732f455fab653fe1427118d905e4d04257.1691339836.git.falcon@tinylab.org/
v1: https://lore.kernel.org/lkml/20230806131921.52453-1-falcon@tinylab.org/

[1]: https://lore.kernel.org/lkml/cover.1691783604.git.falcon@tinylab.org/

Zhangjin Wu (2):
  tools/nolibc: let sys_brk, sys_mmap and sys_mmap2 return long
  tools/nolibc: fix up size inflate regression

 tools/include/nolibc/arch-s390.h |  4 +--
 tools/include/nolibc/sys.h       | 43 +++++++++++++-------------------
 2 files changed, 20 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v6 1/2] tools/nolibc: let sys_brk, sys_mmap and sys_mmap2 return long

[Zhangjin Wu]

Firstly, since the sys_* functions are internally used by our library
routines, it is ok to let them preserve the 'long' return type of
my_syscall<N> macros, that means not necessary to return pointer like
their library routines do.

Secondly, in order to avoid the size inflating issues introduced by the
sign extension, it is better to let __sysret() only accept integer input
types, to do so, we must let all of the sys_* functions not return
pointers.

There are only three sys_* functions which return pointer, let's make
them return 'long' instead of pointer.

Link: https://lore.kernel.org/lkml/20230809221743.83107-1-falcon@tinylab.org/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/arch-s390.h |  4 ++--
 tools/include/nolibc/sys.h       | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/include/nolibc/arch-s390.h b/tools/include/nolibc/arch-s390.h
index 5d60fd43f883..6396c2a6bc3a 100644
--- a/tools/include/nolibc/arch-s390.h
+++ b/tools/include/nolibc/arch-s390.h
@@ -160,7 +160,7 @@ struct s390_mmap_arg_struct {
 };
 
 static __attribute__((unused))
-void *sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
+long sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
 	       off_t offset)
 {
 	struct s390_mmap_arg_struct args = {
@@ -172,7 +172,7 @@ void *sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
 		.offset = (unsigned long)offset
 	};
 
-	return (void *)my_syscall1(__NR_mmap, &args);
+	return my_syscall1(__NR_mmap, &args);
 }
 #define sys_mmap sys_mmap
 
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 833d6c5e86dc..a28e7fbff448 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -74,9 +74,9 @@ long __sysret(unsigned long ret)
  */
 
 static __attribute__((unused))
-void *sys_brk(void *addr)
+long sys_brk(void *addr)
 {
-	return (void *)my_syscall1(__NR_brk, addr);
+	return my_syscall1(__NR_brk, addr);
 }
 
 static __attribute__((unused))
@@ -89,12 +89,12 @@ static __attribute__((unused))
 void *sbrk(intptr_t inc)
 {
 	/* first call to find current end */
-	void *ret = sys_brk(0);
+	void *ret = (void *)sys_brk(0);
 
-	if (ret && sys_brk(ret + inc) == ret + inc)
+	if (ret && (void *)sys_brk(ret + inc) == ret + inc)
 		return ret + inc;
 
-	return (void *)__sysret(-ENOMEM);
+	return (void *)__sysret((long)-ENOMEM);
 }
 
 
@@ -658,7 +658,7 @@ int mknod(const char *path, mode_t mode, dev_t dev)
 
 #ifndef sys_mmap
 static __attribute__((unused))
-void *sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
+long sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
 	       off_t offset)
 {
 	int n;
@@ -670,7 +670,7 @@ void *sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
 	n = __NR_mmap;
 #endif
 
-	return (void *)my_syscall6(n, addr, length, prot, flags, fd, offset);
+	return my_syscall6(n, addr, length, prot, flags, fd, offset);
 }
 #endif
 
@@ -682,7 +682,7 @@ void *sys_mmap(void *addr, size_t length, int prot, int flags, int fd,
 static __attribute__((unused))
 void *mmap(void *addr, size_t length, int prot, int flags, int fd, off_t offset)
 {
-	return (void *)__sysret((unsigned long)sys_mmap(addr, length, prot, flags, fd, offset));
+	return (void *)__sysret(sys_mmap(addr, length, prot, flags, fd, offset));
 }
 
 static __attribute__((unused))
-- 
2.25.1

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

[Zhangjin Wu]

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(-)

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
-- 
2.25.1

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

[Willy Tarreau]

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

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@1wt.eu/#R
[2]: https://lore.kernel.org/lkml/20230813132620.19411-1-falcon@tinylab.org/

> Willy

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

[Willy Tarreau]

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

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

[Willy Tarreau]

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

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

[Zhangjin Wu]

> 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

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

[Willy Tarreau]

On Sat, Aug 12, 2023 at 05:49:36AM +0800, Zhangjin Wu wrote:
> After these two patches, will send the proposed my_syscall() patchset
> tomorrow, it can even further reduce more type conversions and therefore
> reduce more binary bytes, here is a preview of the testing result:
> 
>    // with the coming my_syscall() patchset, sys_* from functionsn to macros
>      i386: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 19250
>    x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21733
(...)
> It can also shrink the whole sys.h from 1171 lines to around 738 lines.

Please, Zhangjin, please. Let's stop constantly speaking about potential
future improvements when the present is broken. It needlessly adds a lot
of noise in the discussion and tends to encourage you to explore areas
that are incompatible with what is required to fix the breakage, and
very likely steers your approach to fixes in a direction that you think
is compatible with such future paths. But as long as existing code is
broken you cannot speculate on how better the next iteration will be,
because it's built on a broken basis. And I would like to remind that
the *only* reason for the current breakage is this attempt to save even
more code lines, that was not a requirement at all in the first place!
Sure it can be fine to remove code when possible, but not at the cost of
trying to force squares to enter round holes like this. The reality is
that *some* syscalls are different and *some* archs are different, and
these differences have to be taken into account, and if we keep exceptions
it's fine.

So let's only speak about this later once the issue is completely solved.

Thanks,
Willy

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

[Zhangjin Wu]

Hi, Willy

> On Sat, Aug 12, 2023 at 05:49:36AM +0800, Zhangjin Wu wrote:
> > After these two patches, will send the proposed my_syscall() patchset
> > tomorrow, it can even further reduce more type conversions and therefore
> > reduce more binary bytes, here is a preview of the testing result:
> > 
> >    // with the coming my_syscall() patchset, sys_* from functionsn to macros
> >      i386: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 19250
> >    x86_64: 160 test(s): 158 passed,   2 skipped,   0 failed => status: warning 21733
> (...)
> > It can also shrink the whole sys.h from 1171 lines to around 738 lines.
> 
> Please, Zhangjin, please. Let's stop constantly speaking about potential
> future improvements when the present is broken. It needlessly adds a lot
> of noise in the discussion and tends to encourage you to explore areas
> that are incompatible with what is required to fix the breakage, and
> very likely steers your approach to fixes in a direction that you think
> is compatible with such future paths. But as long as existing code is
> broken you cannot speculate on how better the next iteration will be,
> because it's built on a broken basis. And I would like to remind that
> the *only* reason for the current breakage is this attempt to save even
> more code lines, that was not a requirement at all in the first place!
> Sure it can be fine to remove code when possible, but not at the cost of
> trying to force squares to enter round holes like this. The reality is
> that *some* syscalls are different and *some* archs are different, and
> these differences have to be taken into account, and if we keep exceptions
> it's fine.
>

Agree very much, that's why I didn't send the new patchset but only send
these two ones about size inflate regression, I don't want to discuss
more than one issue at a time either (and you also have shared this idea
several times) ;-)

The progress and preview data here is only because the patch 1/2 [1] is
an important preparation of the new patchset, the data here is more or
less providing a selling point why we need patch 1/2, I have explained
it in this reply [2]. Of course, we can roll them back directly, and If
we do need sys_brk/mmap return 'long', we can revert the rolling-back
and apply patch 1/2.

    [PATCH v6 1/2] tools/nolibc: let sys_brk, sys_mmap and sys_mmap2 return long

> So let's only speak about this later once the issue is completely solved.
>

Ok, it is the right direction.

Best regards,
Zhangjin
---
[1]: https://lore.kernel.org/lkml/82b584cbda5cee8d5318986644a2a64ba749a098.1691788036.git.falcon@tinylab.org/
[2]: https://lore.kernel.org/lkml/20230813132620.19411-1-falcon@tinylab.org/

> Thanks,
> Willy