| Message ID | 92770843-66C8-471B-BA9C-DA46E92817B9@akamai.com |
|---|---|
| State | New |
| Headers | show |
On 1 July 2016 at 12:59, Wirth, Allan <awirth@akamai.com> wrote: > Linux on X86_64 does not use sel_arg_struct for select(), the args are > passed directly. This patch switches a define so X86_64 uses the correct > calling convention. > > Signed-off-by: Allan Wirth <awirth@akamai.com> > --- > linux-user/syscall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 8bf6205..209b2a7 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -8002,7 +8002,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > break; > #if defined(TARGET_NR_select) > case TARGET_NR_select: > -#if defined(TARGET_S390X) || defined(TARGET_ALPHA) > +#if defined(TARGET_S390X) || defined(TARGET_ALPHA) || defined(TARGET_X86_64) > ret = do_select(arg1, arg2, arg3, arg4, arg5); > #else > { There is a cleaner approach which we should use to fix this: see my comments in reply to this recent patch trying to do a similar thing: https://patchwork.kernel.org/patch/9185927/ thanks -- PMM
Thanks for the feedback. I didn’t find that patch before when I searched, so apologies for the duplicate submission. The proposed fix certainly does seem cleaner and more general. Does it imply though that this patch is incorrect? It fixes the emulation bug in my use case, and AFAICT does not introduce new emulation bugs. Cheers, Allan Wirth On 7/1/16, 9:35 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote: >On 1 July 2016 at 12:59, Wirth, Allan <awirth@akamai.com> wrote: >> Linux on X86_64 does not use sel_arg_struct for select(), the args are >> passed directly. This patch switches a define so X86_64 uses the correct >> calling convention. >> >> Signed-off-by: Allan Wirth <awirth@akamai.com> >> --- >> linux-user/syscall.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 8bf6205..209b2a7 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -8002,7 +8002,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, >> break; >> #if defined(TARGET_NR_select) >> case TARGET_NR_select: >> -#if defined(TARGET_S390X) || defined(TARGET_ALPHA) >> +#if defined(TARGET_S390X) || defined(TARGET_ALPHA) || defined(TARGET_X86_64) >> ret = do_select(arg1, arg2, arg3, arg4, arg5); >> #else >> { > >There is a cleaner approach which we should use to fix this: >see my comments in reply to this recent patch trying to do >a similar thing: >https://patchwork.kernel.org/patch/9185927/ > >thanks >-- PMM
On 1 July 2016 at 16:34, Wirth, Allan <awirth@akamai.com> wrote: > Thanks for the feedback. I didn’t find that patch before when I searched, so > apologies for the duplicate submission. > > The proposed fix certainly does seem cleaner and more general. Does it > imply though that this patch is incorrect? It fixes the emulation bug > in my use case, and AFAICT does not introduce new emulation bugs. Well, it depends what you mean by "incorrect". It's pretty common in dealing with a large and old code base to find good opportunities for small refactorings when you investigate a bug. If we allow bugs to be fixed with the smallest and most expedient change, then problems gradually pile up and the codebase becomes unmaintainable. So we often ask patch submitters to do a bit of cleanup in the process of fixing their bug. In this case, fixing the bug in the way that I suggest will fix it for all architectures, not just x86-64, improve the code by deleting an #ifdef, and remove a trap that will otherwise be waiting for the next new architecture that has support contributed to it. thanks -- PMM
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 8bf6205..209b2a7 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -8002,7 +8002,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, break; #if defined(TARGET_NR_select) case TARGET_NR_select: -#if defined(TARGET_S390X) || defined(TARGET_ALPHA) +#if defined(TARGET_S390X) || defined(TARGET_ALPHA) || defined(TARGET_X86_64) ret = do_select(arg1, arg2, arg3, arg4, arg5); #else {
Linux on X86_64 does not use sel_arg_struct for select(), the args are passed directly. This patch switches a define so X86_64 uses the correct calling convention. Signed-off-by: Allan Wirth <awirth@akamai.com> --- linux-user/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 1.9.1