[PATCH] Possible patch for __syscall_cp

[PATCH] Possible patch for __syscall_cp

[Nicholas Wilson]

Hi,

I have here a patch, which may or not be correct - I'm not confident.

In __syscall_cp.c, there's a call to __syscall which deliberately *disables* macro expansion, forcing the call to go via the __syscall function. On Wasm, I don't currently provide this function, instead I'm using the macros for __syscall to redirect via the __syscall<N> functions.

This call in __syscall_cp looks to be practically the only place in the whole of Musl where macro expansion is prevented for __syscall (other than in src/internal/syscall.h itself, and arch/mips/syscall_arch.h). So on those grounds it's a bit suspicious, given that every other caller of __syscall uses the macros from internal/syscall.h.

If there is a rationale, I could just define __syscall() in arch/wasm/syscall_arch.h - but I'd rather not if it's not meant to be called. Would it be possible instead to apply the patch below, and allow macro expansion?

I can't see a risk, although I don't understand the cancellation-point implementation very well. There's no recursion possible, since plain __syscall() never redirects to any of the __syscall_cp machinery, so expansion oughtn't to cause problems on any archs?

Thanks,
Nick

diff --git a/src/thread/__syscall_cp.c b/src/thread/__syscall_cp.c
index 09a2be84..7b870faa 100644
--- a/src/thread/__syscall_cp.c
+++ b/src/thread/__syscall_cp.c
@@ -8,7 +8,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);

Re: [PATCH] Possible patch for __syscall_cp

[Szabolcs Nagy]

* Nicholas Wilson <nicholas.wilson@realvnc.com> [2018-02-13 14:28:32 +0000]:
> 
> In __syscall_cp.c, there's a call to __syscall which deliberately *disables* macro expansion, forcing the call to go via the __syscall function. On Wasm, I don't currently provide this function, instead I'm using the macros for __syscall to redirect via the __syscall<N> functions.
> 
> This call in __syscall_cp looks to be practically the only place in the whole of Musl where macro expansion is prevented for __syscall (other than in src/internal/syscall.h itself, and arch/mips/syscall_arch.h). So on those grounds it's a bit suspicious, given that every other caller of __syscall uses the macros from internal/syscall.h.
> 
> If there is a rationale, I could just define __syscall() in arch/wasm/syscall_arch.h - but I'd rather not if it's not meant to be called. Would it be possible instead to apply the patch below, and allow macro expansion?
> 
> I can't see a risk, although I don't understand the cancellation-point implementation very well. There's no recursion possible, since plain __syscall() never redirects to any of the __syscall_cp machinery, so expansion oughtn't to cause problems on any archs?
> 

i think your patch is ok (__syscall6 should behave the same
way as __syscall other than the inlining), but you can fix
it for your target only by adding

static inline long __syscall(long n, long a, long b, long c, long d, long e, long f)
{
	return __syscall6(n,a,b,c,d,e,f);
}

to syscall_arch.h

> diff --git a/src/thread/__syscall_cp.c b/src/thread/__syscall_cp.c
> index 09a2be84..7b870faa 100644
> --- a/src/thread/__syscall_cp.c
> +++ b/src/thread/__syscall_cp.c
> @@ -8,7 +8,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);

Re: [PATCH] Possible patch for __syscall_cp

[Nicholas Wilson]

On 13 February 2018 14:49, Szabolcs Nagy wrote:
> i think your patch is ok (__syscall6 should behave the same
> way as __syscall other than the inlining), but you can fix
> it for your target only by adding
>
> static inline long __syscall(long n, long a, long b, long c, long d, long e, long f)
> {
>        return __syscall6(n,a,b,c,d,e,f);
> }
>
> to syscall_arch.h

Yes, that's the approach I was thinking of when I mentioned we could just implement __syscall. In fact, _syscall is declared as varargs, so it would have to be:

static inline long __syscall(long n, ...)
{
  va_list va;
  va_start(va, n);
  long a = va_arg(va, long);
  ... etc
  long f = va_arg(va, long);
  return __syscall6(n, a, b, ..., f);
}

The question really is - would you prefer archs to define __syscall that way, or would you rather patch __syscall_cp to allow macro expansion? In your opinion, which is cleaner? If you don't want to see a shim like that in the archs, would you consider applying the __syscall_cp patch for us?

Thanks for the feedback,
Nick

Re: [PATCH] Possible patch for __syscall_cp

[Rich Felker]

On Wed, Feb 14, 2018 at 12:09:18PM +0000, Nicholas Wilson wrote:
> On 13 February 2018 14:49, Szabolcs Nagy wrote:
> > i think your patch is ok (__syscall6 should behave the same
> > way as __syscall other than the inlining), but you can fix
> > it for your target only by adding
> >
> > static inline long __syscall(long n, long a, long b, long c, long d, long e, long f)
> > {
> >        return __syscall6(n,a,b,c,d,e,f);
> > }
> >
> > to syscall_arch.h
> 
> Yes, that's the approach I was thinking of when I mentioned we could
> just implement __syscall. In fact, _syscall is declared as varargs,
> so it would have to be:
> 
> static inline long __syscall(long n, ...)
> {
>   va_list va;
>   va_start(va, n);
>   long a = va_arg(va, long);
>   ... etc
>   long f = va_arg(va, long);
>   return __syscall6(n, a, b, ..., f);
> }
> 
> The question really is - would you prefer archs to define __syscall
> that way, or would you rather patch __syscall_cp to allow macro
> expansion? In your opinion, which is cleaner? If you don't want to
> see a shim like that in the archs, would you consider applying the
> __syscall_cp patch for us?

The macro suppression is intentional so that the stub version of
__syscall_cp (only used in static linking without thread cancellation
linked) is just a tail call to __syscall rather than a bunch of
register shuffling. Being that the whole point of the stub version is
to avoid the size cost of cancellation logic in small static linked
programs, it doesn't make sense to make a change here.

musl ports should define an external (but hidden) function __syscall
replacing (the empty) src/internal/syscall.c. Perhaps that file should
actually be non-empty and look like the above, which would work
(albeit suboptimally) for any arch that doesn't make an external call
for __syscall6.

Unfortunately there are some messy questions about how many arguments
__syscall is called with and whether it's actually valid for it to be
defined with a C function using va_arg rather than in asm. These are
independent of whether we apply your patch or not, I think, and they
should probably be fixed by ensuring that any place where the external
__syscall is called actually passes 6 arguments, by adding dummy zero
arguments to the __syscallN macros in the SYSCALL_NO_INLINE case, and
in the syscall_arch.h files that use it.

This is all much messier than it should be...

Rich

Re: [PATCH] Possible patch for __syscall_cp

[Nicholas Wilson]

On 21 February 2018 02:03, Rich Felker wrote:
> The macro suppression is intentional so that the stub version of
> __syscall_cp (only used in static linking without thread cancellation
> linked) is just a tail call to __syscall rather than a bunch of
> register shuffling. Being that the whole point of the stub version is
> to avoid the size cost of cancellation logic in small static linked
> programs, it doesn't make sense to make a change here.

OK, that makes sense. Thanks!

> musl ports should define an external (but hidden) function __syscall
> replacing (the empty) src/internal/syscall.c. Perhaps that file should
> actually be non-empty and look like the above, which would work
> (albeit suboptimally) for any arch that doesn't make an external call
> for __syscall6.
>
> Unfortunately there are some messy questions about how many arguments
> __syscall is called with and whether it's actually valid for it to be
> defined with a C function using va_arg rather than in asm. These are
> independent of whether we apply your patch or not, I think, and they
> should probably be fixed by ensuring that any place where the external
> __syscall is called actually passes 6 arguments, by adding dummy zero
> arguments to the __syscallN macros in the SYSCALL_NO_INLINE case, and
> in the syscall_arch.h files that use it.

I think that makes sense to me too. I've added the required stub to the Wasm port in this commit:
https://github.com/NWilson/musl/commit/7cd91fc86358076e601783144f242824afd984ec

I guess it makes sense for Wasm to provide this stub itself, since other arches won't be using it, and it's not likely to be useful to many other arches either.

Regarding passing the six args to __syscall, I think that does work already; __syscall_cp is the only place where __syscall is used with macro suppression, and it does use six args. All the other places go via the macros which are able pad the arguments to the correct length.

I'm happy with things staying as they are then in upstream Musl.

Thanks for the help,
Nick