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

[Willy Tarreau]

Hi Zhangjin,

On Tue, Aug 08, 2023 at 04:04:05AM +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, comparison to -MAX_ERRNO inflicts on all integer returns where
> we could previously keep a simple sign comparison, let's use a new
> __is_pointer() macro suggested by David Laight to limit the comparison
> to -MAX_ERRNO (2) only for pointer returns and preserve a simple sign
> comparison for integer returns as before. The __builtin_choose_expr()
> is suggested by David Laight to choose different comparisons based on
> the types to share code.
> 
> Thirdly, fix up the following warning by an explicit conversion and let
> __sysret() be able to accept the (void *) type of argument:
> 
>     sysroot/powerpc/include/sys.h: In function 'sbrk':
>     sysroot/powerpc/include/sys.h:104:16: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>       104 |         return (void *)__sysret(-ENOMEM);
> 
> Fourthly, to further workaround the argument type with 'const' flag,
> must use __auto_type for gcc >= 11.0 and __typeof__((arg) + 0) suggested
> by David Laight for old gcc versions.
(...)
> tools/include/nolibc/sys.h | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 59 insertions(+), 15 deletions(-)

Quite frankly, even if it can be looked at as a piece of art, I don't
like it. It's overkill for what we need and it brings in several tricky
macros that we don't need and that require a link to their analysis so
that nobody breaks them by accident. I mean, if one day we need them,
okay we know we can find them, they're perfect for certain use cases.
But all this just to avoid a ternary operation is far too much IMHO.
That's without counting on the compiler tricks to use the ugly
__auto_type when available, and the macro names which continue to
possibly interact with user code.

And if you remember, originally you proposed to factor the SET_ERRNO()
stuff in every syscall in order to "simplify the code and improve
maintainability". It's clear that we've long abandonned that goal here.
If we had no other choice, I'd rather roll back to the clean, readable
and trustable SET_ERRNO() in every syscall!

So I just restarted from what I proposed the other day, using a ternary
operator as I suggested in order to address the const case, and it gives
me the following patch, which is way simpler and still a bit readable.
It's made of two nested (?:) :
  - the first one to determine if we have to check for the sign or
    against -MAX_ERRNO to detect an error (depends on the arg's
    signedness)
  - the second one to return either the argument as-is or -1.

The only two tricks are that (typeof(arg))-1 is compared to 1 instead of
zero so that gcc doesn't complain that we're comparing against a null
pointer, and similarly we compare arg+1 to 1 instead of arg to 0 for the
negative case, and that's all. It gives me the expected code and output
from gcc-4.7 to 12.3, and clang-13.

I've checked against your version and it's always exactly the same (in
fact to be more precise sometimes it's 1-2 bytes smaller but that's only
due to the compiler taking liberties with the code ordering, it could as
well have done it the other way around, though it did not this time):

 26144 zhangjin-v5/nolibc-test--Os-arm64     | 26144 willy/nolibc-test--Os-arm64
 23340 zhangjin-v5/nolibc-test--Os-armv5     | 23340 willy/nolibc-test--Os-armv5
 23232 zhangjin-v5/nolibc-test--Os-armv7     | 23232 willy/nolibc-test--Os-armv7
 17508 zhangjin-v5/nolibc-test--Os-armv7t    | 17508 willy/nolibc-test--Os-armv7t
 19674 zhangjin-v5/nolibc-test--Os-i386      | 19673 willy/nolibc-test--Os-i386
 19821 zhangjin-v5/nolibc-test--Os-i586      | 19820 willy/nolibc-test--Os-i586
 23084 zhangjin-v5/nolibc-test--Os-loongarch | 23084 willy/nolibc-test--Os-loongarch
 23404 zhangjin-v5/nolibc-test--Os-mips      | 23404 willy/nolibc-test--Os-mips
 27108 zhangjin-v5/nolibc-test--Os-ppc32     | 27108 willy/nolibc-test--Os-ppc32
 27652 zhangjin-v5/nolibc-test--Os-ppc64     | 27652 willy/nolibc-test--Os-ppc64
 27652 zhangjin-v5/nolibc-test--Os-ppc64le   | 27652 willy/nolibc-test--Os-ppc64le
 19356 zhangjin-v5/nolibc-test--Os-riscv     | 19356 willy/nolibc-test--Os-riscv
 22152 zhangjin-v5/nolibc-test--Os-s390      | 22152 willy/nolibc-test--Os-s390
 22300 zhangjin-v5/nolibc-test--Os-x86_64    | 22299 willy/nolibc-test--Os-x86_64

Unless there's any objection, I'll queue this one. And if __sysret()
annoys us again in the future I might very well revert that simplification.

Any question about the patch ?

Thanks,
Willy

---