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
