Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Zhangjin Wu <falcon@tinylab.org>
To:     w@1wt.eu
Cc:     arnd@arndb.de, david.laight@aculab.com, falcon@tinylab.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        thomas@t-8ch.de
Subject: Re: [PATCH] tools/nolibc: fix up size inflate regression
Date:   Mon,  7 Aug 2023 00:36:49 +0800
Message-Id: <20230806163649.185843-1-falcon@tinylab.org>
In-Reply-To: <20230806134348.GA19145@1wt.eu>
References: <20230806134348.GA19145@1wt.eu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrgz:qybglogicsvrgz5a-1
Precedence: bulk

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