| Message ID | 20240908071600.430410-1-mjt@tls.msk.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [trvivial] linux-user/syscall.c: replace function pointers for flock64 fcntl with macros | expand |
On 9/8/24 00:16, Michael Tokarev wrote: > For 32bit ARM fcntl64 syscall there are 2 possible argument types, > depending on cpu_env->eabi. For other architectures, it is plain > struct flock64 in all cases. In order to solve this, old code > used to take address of the conversion function and and run it > through this pointer. Instead, introduce 2 helper macros for > the flock64 conversion, and define them in the same block where > the oabi conversion functions are defined, and use these > helpers directly in the actual code, making each part more > self-contained and easier to read. > > Also add comment to the block of code which itroduces the oabi > conversion functions. > > Note also there was an inconsistency in the old code: different > the differences in single place we eliminate this difference. > > While at it, replace tabs with spaces in nearby code. > > Signed-off-by: Michael Tokarev<mjt@tls.msk.ru> > --- > linux-user/syscall.c | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) Why do you think this is an improvement? What was wrong with the function pointers? r~
08.09.2024 20:39, Richard Henderson wrote: > On 9/8/24 00:16, Michael Tokarev wrote: >> For 32bit ARM fcntl64 syscall there are 2 possible argument types, >> depending on cpu_env->eabi. For other architectures, it is plain >> struct flock64 in all cases. In order to solve this, old code >> used to take address of the conversion function and and run it >> through this pointer. Instead, introduce 2 helper macros for >> the flock64 conversion, and define them in the same block where >> the oabi conversion functions are defined, and use these >> helpers directly in the actual code, making each part more >> self-contained and easier to read. >> >> Also add comment to the block of code which itroduces the oabi >> conversion functions. >> >> Note also there was an inconsistency in the old code: different >> the differences in single place we eliminate this difference. >> >> While at it, replace tabs with spaces in nearby code. >> >> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru> >> --- >> linux-user/syscall.c | 40 +++++++++++++++++++++++----------------- >> 1 file changed, 23 insertions(+), 17 deletions(-) > > Why do you think this is an improvement? It just feels more natural, so to say. > What was wrong with the function pointers? Not exactly wrong. It just hurts my eyes when I see an address is taken of a function marked `inline` (though I understand well this keyword is just a hint and the compiler is free to omit inlining). Also the typedefs are a bit ugly. I come through this place when removing remnants of LFS, and disliked this way. Thanks, /mjt
On 9/8/24 22:26, Michael Tokarev wrote: >> Why do you think this is an improvement? > > It just feels more natural, so to say. > >> What was wrong with the function pointers? > > Not exactly wrong. It just hurts my eyes when I see an address > is taken of a function marked `inline` I'm certainly happy to fix that! > (though I understand well > this keyword is just a hint and the compiler is free to omit > inlining). Also the typedefs are a bit ugly. I think the macro is uglier than the typedef. r~
On Mon, 9 Sept 2024 at 17:19, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 9/8/24 22:26, Michael Tokarev wrote: > >> Why do you think this is an improvement? > > > > It just feels more natural, so to say. > > > >> What was wrong with the function pointers? > > > > Not exactly wrong. It just hurts my eyes when I see an address > > is taken of a function marked `inline` > > I'm certainly happy to fix that! > > > (though I understand well > > this keyword is just a hint and the compiler is free to omit > > inlining). Also the typedefs are a bit ugly. > > I think the macro is uglier than the typedef. This was my opinion also. Plus the compiler generates reasonable code with our current source, and the code path isn't a hot one. thanks -- PMM
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index d418e2864a..03250316df 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -6880,10 +6880,12 @@ 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 +/* + * For 32bit arm flock64 syscall there are 2 variants of target_flock + * depending on eabi + */ + struct target_oabi_flock64 { abi_short l_type; abi_short l_whence; @@ -6935,6 +6937,19 @@ static inline abi_long copy_to_user_oabi_flock64(abi_ulong target_flock_addr, unlock_user_struct(target_fl, target_flock_addr, 1); return 0; } + +#define copy_from_user_flock64_2abi(cpu_env, fl, target_flock_addr) \ + (cpu_env->eabi ? copy_from_user_flock64(fl, target_flock_addr) : \ + copy_from_user_oabi_flock64(fl, target_flock_addr)) +#define copy_to_user_flock64_2abi(cpu_env, target_flock_addr, fl) \ + (cpu_env->eabi ? copy_to_user_flock64(target_flock_addr, fl) \ + copy_to_user_oabi_flock64(target_flock_addr, fl)) +#else + +#define copy_from_user_flock64_2abi(cpu_env, fl, target_flock_addr) \ + copy_from_user_flock64(fl, target_flock_addr) +#define copy_to_user_flock64_2abi(cpu_env, target_flock_addr, fl) \ + copy_to_user_flock64(target_flock_addr, fl) #endif static inline abi_long copy_from_user_flock64(struct flock *fl, @@ -12402,15 +12417,6 @@ 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; - } -#endif cmd = target_to_host_fcntl_cmd(arg2); if (cmd == -TARGET_EINVAL) { @@ -12419,24 +12425,24 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, switch(arg2) { case TARGET_F_GETLK64: - ret = copyfrom(&fl, arg3); + ret = copy_from_user_flock64_2abi(cpu_env, &fl, arg3); if (ret) { break; } ret = get_errno(safe_fcntl(arg1, cmd, &fl)); if (ret == 0) { - ret = copyto(arg3, &fl); + ret = copy_to_user_flock64_2abi(cpu_env, arg3, &fl); } - break; + break; case TARGET_F_SETLK64: case TARGET_F_SETLKW64: - ret = copyfrom(&fl, arg3); + ret = copy_from_user_flock64_2abi(cpu_env, &fl, arg3); if (ret) { break; } ret = get_errno(safe_fcntl(arg1, cmd, &fl)); - break; + break; default: ret = do_fcntl(arg1, arg2, arg3); break;
For 32bit ARM fcntl64 syscall there are 2 possible argument types, depending on cpu_env->eabi. For other architectures, it is plain struct flock64 in all cases. In order to solve this, old code used to take address of the conversion function and and run it through this pointer. Instead, introduce 2 helper macros for the flock64 conversion, and define them in the same block where the oabi conversion functions are defined, and use these helpers directly in the actual code, making each part more self-contained and easier to read. Also add comment to the block of code which itroduces the oabi conversion functions. Note also there was an inconsistency in the old code: different the differences in single place we eliminate this difference. While at it, replace tabs with spaces in nearby code. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- linux-user/syscall.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)