| Message ID | 20180601073050.8054-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user: Begin splitting do_syscall | expand |
Le 01/06/2018 à 09:30, Richard Henderson a écrit : > There was supposed to be a single point of return for do_syscall > so that tracing works properly. However, there are a few bugs > in that area. It is significantly simpler to simply split out > an inner function to enforce this. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/syscall.c | 89 +++++++++++++++++++++++++++----------------- > 1 file changed, 54 insertions(+), 35 deletions(-) Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On 06/01/2018 02:30 AM, Richard Henderson wrote: > There was supposed to be a single point of return for do_syscall > so that tracing works properly. However, there are a few bugs > in that area. It is significantly simpler to simply split out > an inner function to enforce this. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/syscall.c | 89 +++++++++++++++++++++++++++----------------- > 1 file changed, 54 insertions(+), 35 deletions(-) > > @@ -7977,28 +7979,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > void *p; > char *fn; > > -#if defined(DEBUG_ERESTARTSYS) > +abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > + abi_long arg2, abi_long arg3, abi_long arg4, > + abi_long arg5, abi_long arg6, abi_long arg7, > + abi_long arg8) > +{ > + CPUState *cpu = ENV_GET_CPU(cpu_env); > + abi_long ret; > + > +#if defined(DEBUG_ERESTARTSYS) Code motion, but... > + /* Debug-only code for exercising the syscall-restart code paths > + * in the per-architecture cpu main loops: restart every syscall > + * the guest makes once before letting it through. > + */ > + { > + static bool flag; > + flag = !flag; > + if (flag) { > + return -TARGET_ERESTARTSYS; > + } > + } > +#endif > +#ifdef DEBUG ...it looks inconsistent to mix '#if defined()' with '#ifdef' in the same function. Worth cleaning up while you do this refactoring? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 06/01/2018 07:00 AM, Eric Blake wrote: > On 06/01/2018 02:30 AM, Richard Henderson wrote: >> There was supposed to be a single point of return for do_syscall >> so that tracing works properly. However, there are a few bugs >> in that area. It is significantly simpler to simply split out >> an inner function to enforce this. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> linux-user/syscall.c | 89 +++++++++++++++++++++++++++----------------- >> 1 file changed, 54 insertions(+), 35 deletions(-) >> > >> @@ -7977,28 +7979,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long >> arg1, >> void *p; >> char *fn; >> -#if defined(DEBUG_ERESTARTSYS) > >> +abi_long do_syscall(void *cpu_env, int num, abi_long arg1, >> + abi_long arg2, abi_long arg3, abi_long arg4, >> + abi_long arg5, abi_long arg6, abi_long arg7, >> + abi_long arg8) >> +{ >> + CPUState *cpu = ENV_GET_CPU(cpu_env); >> + abi_long ret; >> + >> +#if defined(DEBUG_ERESTARTSYS) > > Code motion, but... > >> + /* Debug-only code for exercising the syscall-restart code paths >> + * in the per-architecture cpu main loops: restart every syscall >> + * the guest makes once before letting it through. >> + */ >> + { >> + static bool flag; >> + flag = !flag; >> + if (flag) { >> + return -TARGET_ERESTARTSYS; >> + } >> + } >> +#endif >> +#ifdef DEBUG > > ...it looks inconsistent to mix '#if defined()' with '#ifdef' in the same > function. Worth cleaning up while you do this refactoring? Yes, I've been trying to remember to fix this as I go, but obviously missed some. r~ >
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index b75dd9a5bc..ebaefebcc2 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7962,13 +7962,15 @@ static int host_to_target_cpu_mask(const unsigned long *host_mask, return 0; } -/* do_syscall() should always have a single exit point at the end so - that actions, such as logging of syscall results, can be performed. - All errnos that do_syscall() returns must be -TARGET_<errcode>. */ -abi_long do_syscall(void *cpu_env, int num, abi_long arg1, - abi_long arg2, abi_long arg3, abi_long arg4, - abi_long arg5, abi_long arg6, abi_long arg7, - abi_long arg8) +/* This is an internal helper for do_syscall so that it is easier + * to have a single return point, so that actions, such as logging + * of syscall results, can be performed. + * All errnos that do_syscall() returns must be -TARGET_<errcode>. + */ +static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, + abi_long arg2, abi_long arg3, abi_long arg4, + abi_long arg5, abi_long arg6, abi_long arg7, + abi_long arg8) { CPUState *cpu = ENV_GET_CPU(cpu_env); abi_long ret; @@ -7977,28 +7979,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, void *p; char *fn; -#if defined(DEBUG_ERESTARTSYS) - /* Debug-only code for exercising the syscall-restart code paths - * in the per-architecture cpu main loops: restart every syscall - * the guest makes once before letting it through. - */ - { - static int flag; - - flag = !flag; - if (flag) { - return -TARGET_ERESTARTSYS; - } - } -#endif - -#ifdef DEBUG - gemu_log("syscall %d", num); -#endif - trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8); - if(do_strace) - print_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6); - switch(num) { case TARGET_NR_exit: /* In old applications this may be used to implement _exit(2). @@ -13101,12 +13081,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, break; } fail: -#ifdef DEBUG - gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret); -#endif - if(do_strace) - print_syscall_ret(num, ret); - trace_guest_user_syscall_ret(cpu, num, ret); return ret; efault: ret = -TARGET_EFAULT; @@ -13115,3 +13089,48 @@ ebadf: ret = -TARGET_EBADF; goto fail; } + +abi_long do_syscall(void *cpu_env, int num, abi_long arg1, + abi_long arg2, abi_long arg3, abi_long arg4, + abi_long arg5, abi_long arg6, abi_long arg7, + abi_long arg8) +{ + CPUState *cpu = ENV_GET_CPU(cpu_env); + abi_long ret; + +#if defined(DEBUG_ERESTARTSYS) + /* Debug-only code for exercising the syscall-restart code paths + * in the per-architecture cpu main loops: restart every syscall + * the guest makes once before letting it through. + */ + { + static bool flag; + flag = !flag; + if (flag) { + return -TARGET_ERESTARTSYS; + } + } +#endif +#ifdef DEBUG + gemu_log("syscall %d", num); +#endif + + trace_guest_user_syscall(cpu, num, arg1, arg2, arg3, arg4, + arg5, arg6, arg7, arg8); + + if (unlikely(do_strace)) { + print_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6); + ret = do_syscall1(cpu_env, num, arg1, arg2, arg3, arg4, + arg5, arg6, arg7, arg8); + print_syscall_ret(num, ret); + } else { + ret = do_syscall1(cpu_env, num, arg1, arg2, arg3, arg4, + arg5, arg6, arg7, arg8); + } + +#ifdef DEBUG + gemu_log(" = " TARGET_ABI_FMT_ld "\n", ret); +#endif + trace_guest_user_syscall_ret(cpu, num, ret); + return ret; +}
There was supposed to be a single point of return for do_syscall so that tracing works properly. However, there are a few bugs in that area. It is significantly simpler to simply split out an inner function to enforce this. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/syscall.c | 89 +++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 35 deletions(-) -- 2.17.0