| Message ID | 20240907065918.376217-1-mjt@tls.msk.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [trivial] linux-user/syscall.c: replace function pointer with a helper macro for fcntl/flock case | expand |
On Sat, 7 Sept 2024 at 08:00, Michael Tokarev <mjt@tls.msk.ru> wrote: > > There's a slight difference in fcntl locking handling on arm and others: > arm has 2 variants of struct flock argument handling, with and without eabi. > For this reason, we currently take address of the conversion function into > a variable, and for arm without eabi case, assign a different conversion > function to this pointer. All functions are declared as inline, - and for > an inline function, it is not clear how one can have an address of it at > all. There's no problem with this. "inline" is just a hint to the compiler to say "this is probably better inlined where you can". For the case where we take the address of a function, that's a case where it can't be inlined. But other places where the function is called directly can be inlined without problems. For functions where are only ever used via function pointers, the "inline" annotation is a bit pointless and we could remove it; arguably for other cases we could also remove it and let the compiler make the call about inline vs not. As it happens the kernel is not inclined to inline these functions even if we ask for it, probably because they're too big for that to be a good idea. > Instead of using a function pointer, use a macro, defined differently for > arm (with arithmetic if based on eabi presence) and for all other targets > (using regular argument conversion function directly). > > While at it, replace tabs with spaces in nearby code. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > linux-user/syscall.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index d418e2864a..786623d395 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -6880,9 +6880,6 @@ static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr, > return 0; > } > > -typedef abi_long from_flock64_fn(struct flock *fl, abi_ulong target_addr); > -typedef abi_long to_flock64_fn(abi_ulong target_addr, const struct flock *fl); > - > #if defined(TARGET_ARM) && TARGET_ABI_BITS == 32 > struct target_oabi_flock64 { > abi_short l_type; > @@ -12402,14 +12399,19 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, > { > int cmd; > struct flock fl; > - from_flock64_fn *copyfrom = copy_from_user_flock64; > - to_flock64_fn *copyto = copy_to_user_flock64; > > #ifdef TARGET_ARM > - if (!cpu_env->eabi) { > - copyfrom = copy_from_user_oabi_flock64; > - copyto = copy_to_user_oabi_flock64; > - } > +# define copyfrom(fl, arg) \ > + (cpu_env->eabi ? \ > + copy_from_user_flock64(fl, arg) : \ > + copy_from_user_oabi_flock64(fl, arg)) > +# define copyto(arg, fl) \ > + (cpu_env->eabi ? \ > + copy_to_user_flock64(arg, fl) : \ > + copy_to_user_oabi_flock64(arg, fl)) > +#else > +# define copyfrom copy_from_user_flock64 > +# define copyto copy_to_user_flock64 > #endif This seems to me to be less readable than the current code. Unless there's a specific performance issue with what we have (which seems unlikely for flock), I would prefer not to macroify things like this. Looking at the generated code, for the non-arm cases the compiler is smart enough to turn the call into a direct call to copy_from_user_flock64 already. > cmd = target_to_host_fcntl_cmd(arg2); > @@ -12427,7 +12429,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, > if (ret == 0) { > ret = copyto(arg3, &fl); > } > - break; > + break; > > case TARGET_F_SETLK64: > case TARGET_F_SETLKW64: > @@ -12436,10 +12438,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, > break; > } > ret = get_errno(safe_fcntl(arg1, cmd, &fl)); > - break; > + break; > default: > ret = do_fcntl(arg1, arg2, arg3); > break; > +#undef copyfrom > +#undef copyto > } > return ret; > } thanks -- PMM
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index d418e2864a..786623d395 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -6880,9 +6880,6 @@ static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr, return 0; } -typedef abi_long from_flock64_fn(struct flock *fl, abi_ulong target_addr); -typedef abi_long to_flock64_fn(abi_ulong target_addr, const struct flock *fl); - #if defined(TARGET_ARM) && TARGET_ABI_BITS == 32 struct target_oabi_flock64 { abi_short l_type; @@ -12402,14 +12399,19 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, { int cmd; struct flock fl; - from_flock64_fn *copyfrom = copy_from_user_flock64; - to_flock64_fn *copyto = copy_to_user_flock64; #ifdef TARGET_ARM - if (!cpu_env->eabi) { - copyfrom = copy_from_user_oabi_flock64; - copyto = copy_to_user_oabi_flock64; - } +# define copyfrom(fl, arg) \ + (cpu_env->eabi ? \ + copy_from_user_flock64(fl, arg) : \ + copy_from_user_oabi_flock64(fl, arg)) +# define copyto(arg, fl) \ + (cpu_env->eabi ? \ + copy_to_user_flock64(arg, fl) : \ + copy_to_user_oabi_flock64(arg, fl)) +#else +# define copyfrom copy_from_user_flock64 +# define copyto copy_to_user_flock64 #endif cmd = target_to_host_fcntl_cmd(arg2); @@ -12427,7 +12429,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, if (ret == 0) { ret = copyto(arg3, &fl); } - break; + break; case TARGET_F_SETLK64: case TARGET_F_SETLKW64: @@ -12436,10 +12438,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, break; } ret = get_errno(safe_fcntl(arg1, cmd, &fl)); - break; + break; default: ret = do_fcntl(arg1, arg2, arg3); break; +#undef copyfrom +#undef copyto } return ret; }
There's a slight difference in fcntl locking handling on arm and others: arm has 2 variants of struct flock argument handling, with and without eabi. For this reason, we currently take address of the conversion function into a variable, and for arm without eabi case, assign a different conversion function to this pointer. All functions are declared as inline, - and for an inline function, it is not clear how one can have an address of it at all. Instead of using a function pointer, use a macro, defined differently for arm (with arithmetic if based on eabi presence) and for all other targets (using regular argument conversion function directly). While at it, replace tabs with spaces in nearby code. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- linux-user/syscall.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)