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

[Zhangjin Wu]

Hi, Willy

> On Sun, Aug 06, 2023 at 09:19:21PM +0800, Zhangjin Wu wrote:
> > As reported and suggested by Willy, the inline __sysret() helper
> > introduces three types of conversions and increases the size:
> 
> Thanks Zhangjin. A few things:
> 
> > --- a/tools/include/nolibc/compiler.h
> > +++ b/tools/include/nolibc/compiler.h
> > @@ -22,4 +22,13 @@
> >  #  define __no_stack_protector __attribute__((__optimize__("-fno-stack-protector")))
> >  #endif /* defined(__has_attribute) */
> >  
> > +/*
> > + * from include/linux/compiler.h
> > + *
> > + * Whether 'type' is a signed type or an unsigned type. Supports scalar types,
> > + * bool and also pointer types.
> > + */
> > +#define is_signed_type(type)   (((type)(-1)) < (type)1)
> > +#define is_unsigned_type(type) (!is_signed_type(type))
> 
> These names may conflict with application's local definitions. And since
> there's a single call place we should probably just inline it instead.
>

Let's remove them and define a new one with __ prefix in sys.h, it is
not readable if we simply 'inline' the comparison in the already complex
macro ;-)

> > +#define __sysret(arg)                                                           \
> > +({                                                                              \
> > +	__typeof__(arg) __sysret_arg = (arg);                                   \
> > +	if (is_signed_type(__typeof__(arg))) {                                  \
> > +		if (__sysret_arg < 0) {                                         \
> > +			SET_ERRNO(-(int)__sysret_arg);                          \
> > +			__sysret_arg = -1L;                                     \
> > +		}                                                               \
> > +	} else {                                                                \
> > +		if ((unsigned long)__sysret_arg >= (unsigned long)-MAX_ERRNO) { \
> > +			SET_ERRNO(-(int)__sysret_arg);                          \
> > +			__sysret_arg = -1L;                                     \
> > +		}                                                               \
> > +	}                                                                       \
> > +	__sysret_arg;                                                           \
> > +})
> 
> I also found during my tests that this one can return a build error if
> the arg has "const" in its type, due to the error assignment. We need
> to think about reworking it as a ternary evaluation, it will be more
> reliable even if less readable. But let's not change this now, I'm on
> the changelog already.
>

The __auto_type in new enough version does work well with 'const', but
for the old version, we need to restoring the 'long' type and the
conversion ;-(

> >  /* Functions in this file only describe syscalls. They're declared static so
> >   * that the compiler usually decides to inline them while still being allowed
> > @@ -94,7 +101,7 @@ void *sbrk(intptr_t inc)
> >  	if (ret && sys_brk(ret + inc) == ret + inc)
> >  		return ret + inc;
> >  
> > -	return (void *)__sysret(-ENOMEM);
> > +	return (void *)__sysret((unsigned long)-ENOMEM);
> 
> I noticed that one as well during my tests, but if we're purist, we're
> supposed to use (void*) for the cast and not (unsigned long), and it
> should allow to get rid of the outer cast.
>

To accept "void *", more conversions are required ...

A new 'ugly' version are ready for review soon, it compiles and get less size
on gcc-4.8 too ;-)

Thanks,
Zhangjin

> Regards,
> Willy