[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e3a6bd5b7a4d4ac2bccbaac21e0fc1a0@AcuMS.aculab.com>
Date: Mon, 14 Aug 2023 12:27:48 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Willy Tarreau' <w@....eu>
CC: 'Zhangjin Wu' <falcon@...ylab.org>,
"arnd@...db.de" <arnd@...db.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"thomas@...ch.de" <thomas@...ch.de>
Subject: RE: [PATCH v5] tools/nolibc: fix up size inflate regression
From: Willy Tarreau
> Sent: 14 August 2023 13:10
>
> Hi David,
>
> On Mon, Aug 14, 2023 at 11:15:51AM +0000, David Laight wrote:
> > From: Zhangjin Wu
> > > Sent: 14 August 2023 11:42
> > ...
> > > [...]
> > > > > > Sure it's not pretty, and I'd rather just go back to SET_ERRNO() to be
> > > > > > honest, because we're there just because of the temptation to remove
> > > > > > lines that were not causing any difficulties :-/
> > > > > >
> > > > > > I think we can do something in-between and deal only with signed returns,
> > > > > > and explicitly place the test for MAX_ERRNO on the two unsigned ones
> > > > > > (brk and mmap). It should look approximately like this:
> > > > > >
> > > > > > #define __sysret(arg) \
> > > > > > ({ \
> > > > > > __typeof__(arg) __sysret_arg = (arg); \
> > > > > > (__sysret_arg < 0) ? ({ /* error ? */ \
> > > > > > SET_ERRNO(-__sysret_arg); /* yes: errno != -ret */ \
> > > > > > ((__typeof__(arg)) -1); /* return -1 */ \
> >
> > I'm pretty sure you don't need the explicit cast.
> > (It would be needed for a pointer type.)
> > Can you use __arg < ? SET_ERRNO(-__arg), -1 : __arg
> >
> > Thinking, maybe it should be:
> >
> > #define __sysret(syscall_fn_args)
> > ({
> > __typeof__(syscall_fn_args) __rval = syscall_fn_args;
> > __rval >= 0 ? __rval : SET_ERRNO(-__rval), -1;
> > })
>
> Yeah almost, since arg is necessarily signed in this version, it's
> just that I manually edited the previous macro in the mail and limited
> the amount of changes to what was necessary. It's just that SET_ERRNO
> only is an instruction, not an expression:
>
> #define SET_ERRNO(v) do { errno = (v); } while (0)
>
> Thus the return value doesn't even pass through it. That's why it was
> so much simpler before. The rationale behind this was to bring the
> ability to completely drop errno for programs where you didn't care
> about it. It's particularly interesting when you don't need any other
> data either as the program gets strunk from a complete section.
Actually something like:
#define SET_ERRNO(v) (errno = -(long)(v), __typeof__(v)-1)
seems to work and allows the errno assignment be removed.
Also works for pointer types (after a different compare).
A quick check with godbolt doesn't show any sign extensions happening.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Powered by blists - more mailing lists