[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Sat, 7 Nov 2015 19:51:32 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] don't wrap __syscall invocation in parenthesis
On Sat, Nov 07, 2015 at 02:29:43PM +0100, Szabolcs Nagy wrote:
> * Petr Hosek <phosek@...omium.org> [2015-11-06 23:51:21 +0000]:
> > This is a cleanup change, when __syscall is defined as a macro, its
> > expansion breaks in cases where the invocation is wrapped in parenthesis.
> > There are 2 such cases in the codebase and this patch fixes those.
>
> well that's the point of the parenthesis to stop macro expansion..
>
> i think x32 breaks otherwise (because of the argument cast hacks)
No, it's actually for 32-bit archs that require even-slot alignment of
64-bit syscall arguments. At the time it was added this was just arm.
See commit 0b6eb2dfb2e84a8a51906e7634f3d5edc230b058. To get rid of
the need to call the actuall __syscall function here, we would need to
add a __syscall7 form on archs that need it. I think we should review
this anyway since __syscall (the function) on most of these archs is
probably actually lacking support for 7-argument passing, causing the
advice argument passed to the kernel to be random junk...
> > From 21864f367028899b541290258711750313e226f5 Mon Sep 17 00:00:00 2001
> > From: Petr Hosek <phosek@...omium.org>
> > Date: Thu, 5 Nov 2015 07:57:23 -0800
> > Subject: [PATCH] don't wrap __syscall invocation in parenthesis
> >
> > when __syscall is defined as a macro, its expansion breaks in
> > cases where the invocation is wrapped in parenthesis.
> > ---
> > src/fcntl/posix_fadvise.c | 2 +-
> > src/thread/__syscall_cp.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/fcntl/posix_fadvise.c b/src/fcntl/posix_fadvise.c
> > index d5360e0..fc1562e 100644
> > --- a/src/fcntl/posix_fadvise.c
> > +++ b/src/fcntl/posix_fadvise.c
> > @@ -4,7 +4,7 @@
> >
> > int posix_fadvise(int fd, off_t base, off_t len, int advice)
> > {
> > - return -(__syscall)(SYS_fadvise, fd, __SYSCALL_LL_O(base),
> > + return -__syscall(SYS_fadvise, fd, __SYSCALL_LL_O(base),
> > __SYSCALL_LL_E(len), advice);
> > }
> >
> > diff --git a/src/thread/__syscall_cp.c b/src/thread/__syscall_cp.c
> > index faf57b1..09878c6 100644
> > --- a/src/thread/__syscall_cp.c
> > +++ b/src/thread/__syscall_cp.c
> > @@ -10,7 +10,7 @@ static long sccp(syscall_arg_t nr,
> > syscall_arg_t u, syscall_arg_t v, syscall_arg_t w,
> > syscall_arg_t x, syscall_arg_t y, syscall_arg_t z)
> > {
> > - return (__syscall)(nr, u, v, w, x, y, z);
> > + return __syscall(nr, u, v, w, x, y, z);
> > }
> >
> > weak_alias(sccp, __syscall_cp_c);
> > --
In the latter case, the function call is purely an optimization; it
yields a trivial tail-call rather than actually expanding out to a
duplicate of the syscall code.
I think the NaCl port of musl should simply have a working __syscall
function provided by src/internal/[whatever]/syscall.s. (Or .c as it
may be.) This would make it uniform with other archs and would avoid
the need to remove this optimization. There may be other ways to
restructure this code that's cleaner anyway, but I'd rather keep
design changes like this separate from the actual porting.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
