| Message ID | Y9Q7BlDc/VX+1SBL@p100 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] linux-user: Improve strace output of pread64() and pwrite64() | expand |
On 1/27/23 10:58, Helge Deller wrote: > Make the strace look nicer for those two syscalls. > > Signed-off-by: Helge Deller<deller@gmx.de> > --- > v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Le 27/01/2023 à 21:58, Helge Deller a écrit : > Make the strace look nicer for those two syscalls. > > Signed-off-by: Helge Deller <deller@gmx.de> > --- > v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier > > diff --git a/linux-user/strace.c b/linux-user/strace.c > index 82dc1a1e20..379536f5c9 100644 > --- a/linux-user/strace.c > +++ b/linux-user/strace.c > @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last) > } > } > > +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64) > +static void > +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name, > + abi_long arg0, abi_long arg1, abi_long arg2, > + abi_long arg3, abi_long arg4, abi_long arg5) > +{ > + if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) { > + arg3 = arg4; > + arg4 = arg5; > + } > + print_syscall_prologue(name); > + print_raw_param("%d", arg0, 0); > + print_pointer(arg1, 0); > + print_raw_param("%d", arg2, 0); > + qemu_log("%lld", (long long)target_offset64(arg3, arg4)); better to use: print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1); Thanks, Laurent
On 1/30/23 10:26, Laurent Vivier wrote: > Le 27/01/2023 à 21:58, Helge Deller a écrit : >> Make the strace look nicer for those two syscalls. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> --- >> v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier >> >> diff --git a/linux-user/strace.c b/linux-user/strace.c >> index 82dc1a1e20..379536f5c9 100644 >> --- a/linux-user/strace.c >> +++ b/linux-user/strace.c >> @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last) >> } >> } >> >> +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64) >> +static void >> +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name, >> + abi_long arg0, abi_long arg1, abi_long arg2, >> + abi_long arg3, abi_long arg4, abi_long arg5) >> +{ >> + if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) { >> + arg3 = arg4; >> + arg4 = arg5; >> + } >> + print_syscall_prologue(name); >> + print_raw_param("%d", arg0, 0); >> + print_pointer(arg1, 0); >> + print_raw_param("%d", arg2, 0); >> + qemu_log("%lld", (long long)target_offset64(arg3, arg4)); > > better to use: > > print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1); I thought of that as well, but that won't work, as print_raw_param() takes an "abi_long" value, which is just a 32-bit value on 32-bit targets. See print_rlimit64(), it's used there with qemu_log() as well. Helge
Le 30/01/2023 à 23:11, Helge Deller a écrit : > On 1/30/23 10:26, Laurent Vivier wrote: >> Le 27/01/2023 à 21:58, Helge Deller a écrit : >>> Make the strace look nicer for those two syscalls. >>> >>> Signed-off-by: Helge Deller <deller@gmx.de> >>> --- >>> v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier >>> >>> diff --git a/linux-user/strace.c b/linux-user/strace.c >>> index 82dc1a1e20..379536f5c9 100644 >>> --- a/linux-user/strace.c >>> +++ b/linux-user/strace.c >>> @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last) >>> } >>> } >>> >>> +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64) >>> +static void >>> +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name, >>> + abi_long arg0, abi_long arg1, abi_long arg2, >>> + abi_long arg3, abi_long arg4, abi_long arg5) >>> +{ >>> + if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) { >>> + arg3 = arg4; >>> + arg4 = arg5; >>> + } >>> + print_syscall_prologue(name); >>> + print_raw_param("%d", arg0, 0); >>> + print_pointer(arg1, 0); >>> + print_raw_param("%d", arg2, 0); >>> + qemu_log("%lld", (long long)target_offset64(arg3, arg4)); >> >> better to use: >> >> print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1); > > I thought of that as well, but that won't work, as print_raw_param() > takes an "abi_long" value, which is just a 32-bit value on 32-bit targets. > See print_rlimit64(), it's used there with qemu_log() as well. Yes, you're right. But even with qemu_log() I would prefer you use "%"PRIu64 rather than %lld. Or better define a print_raw_param64() (or similar) and update print_fallocate(), print_truncate64() and print_ftruncate64(). Thanks, Laurent
On 1/31/23 12:04, Laurent Vivier wrote: > Le 30/01/2023 à 23:11, Helge Deller a écrit : >> On 1/30/23 10:26, Laurent Vivier wrote: >>> Le 27/01/2023 à 21:58, Helge Deller a écrit : >>>> Make the strace look nicer for those two syscalls. >>>> >>>> Signed-off-by: Helge Deller <deller@gmx.de> >>>> --- >>>> v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier >>>> >>>> diff --git a/linux-user/strace.c b/linux-user/strace.c >>>> index 82dc1a1e20..379536f5c9 100644 >>>> --- a/linux-user/strace.c >>>> +++ b/linux-user/strace.c >>>> @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last) >>>> } >>>> } >>>> >>>> +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64) >>>> +static void >>>> +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name, >>>> + abi_long arg0, abi_long arg1, abi_long arg2, >>>> + abi_long arg3, abi_long arg4, abi_long arg5) >>>> +{ >>>> + if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) { >>>> + arg3 = arg4; >>>> + arg4 = arg5; >>>> + } >>>> + print_syscall_prologue(name); >>>> + print_raw_param("%d", arg0, 0); >>>> + print_pointer(arg1, 0); >>>> + print_raw_param("%d", arg2, 0); >>>> + qemu_log("%lld", (long long)target_offset64(arg3, arg4)); >>> >>> better to use: >>> >>> print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1); >> >> I thought of that as well, but that won't work, as print_raw_param() >> takes an "abi_long" value, which is just a 32-bit value on 32-bit targets. >> See print_rlimit64(), it's used there with qemu_log() as well. > > Yes, you're right. > > But even with qemu_log() I would prefer you use "%"PRIu64 rather than %lld. > Or better define a print_raw_param64() (or similar) and update print_fallocate(), print_truncate64() and print_ftruncate64(). As adding such define is a unrelated change to this patch, I'd propose that I send a follow-up patch on top of this one which adds print_raw_param64() (or similar) and replaces all usages of qemu_log() with 64-bit values and use "%"PRIu64 then. Ok? If yes, should that define include the last 0/1 parameter to print the "," ? I think so, because then it's consistent with print_raw_param(). Helge
Le 31/01/2023 à 14:08, Helge Deller a écrit : > On 1/31/23 12:04, Laurent Vivier wrote: >> Le 30/01/2023 à 23:11, Helge Deller a écrit : >>> On 1/30/23 10:26, Laurent Vivier wrote: >>>> Le 27/01/2023 à 21:58, Helge Deller a écrit : >>>>> Make the strace look nicer for those two syscalls. >>>>> >>>>> Signed-off-by: Helge Deller <deller@gmx.de> >>>>> --- >>>>> v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier >>>>> >>>>> diff --git a/linux-user/strace.c b/linux-user/strace.c >>>>> index 82dc1a1e20..379536f5c9 100644 >>>>> --- a/linux-user/strace.c >>>>> +++ b/linux-user/strace.c >>>>> @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last) >>>>> } >>>>> } >>>>> >>>>> +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64) >>>>> +static void >>>>> +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name, >>>>> + abi_long arg0, abi_long arg1, abi_long arg2, >>>>> + abi_long arg3, abi_long arg4, abi_long arg5) >>>>> +{ >>>>> + if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) { >>>>> + arg3 = arg4; >>>>> + arg4 = arg5; >>>>> + } >>>>> + print_syscall_prologue(name); >>>>> + print_raw_param("%d", arg0, 0); >>>>> + print_pointer(arg1, 0); >>>>> + print_raw_param("%d", arg2, 0); >>>>> + qemu_log("%lld", (long long)target_offset64(arg3, arg4)); >>>> >>>> better to use: >>>> >>>> print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1); >>> >>> I thought of that as well, but that won't work, as print_raw_param() >>> takes an "abi_long" value, which is just a 32-bit value on 32-bit targets. >>> See print_rlimit64(), it's used there with qemu_log() as well. >> >> Yes, you're right. >> >> But even with qemu_log() I would prefer you use "%"PRIu64 rather than %lld. >> Or better define a print_raw_param64() (or similar) and update print_fallocate(), >> print_truncate64() and print_ftruncate64(). > > As adding such define is a unrelated change to this patch, I'd propose that I send a follow-up > patch on top of this one which adds print_raw_param64() (or similar) and replaces all > usages of qemu_log() with 64-bit values and use "%"PRIu64 then. > Ok? yes > If yes, should that define include the last 0/1 parameter to print the "," ? > I think so, because then it's consistent with print_raw_param(). you're right. Thanks, Laurent > > Helge >
diff --git a/linux-user/strace.c b/linux-user/strace.c index 82dc1a1e20..379536f5c9 100644 --- a/linux-user/strace.c +++ b/linux-user/strace.c @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last) } } +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64) +static void +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name, + abi_long arg0, abi_long arg1, abi_long arg2, + abi_long arg3, abi_long arg4, abi_long arg5) +{ + if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) { + arg3 = arg4; + arg4 = arg5; + } + print_syscall_prologue(name); + print_raw_param("%d", arg0, 0); + print_pointer(arg1, 0); + print_raw_param("%d", arg2, 0); + qemu_log("%lld", (long long)target_offset64(arg3, arg4)); + print_syscall_epilogue(name); +} +#endif + static void print_prlimit64(CPUArchState *cpu_env, const struct syscallname *name, abi_long arg0, abi_long arg1, abi_long arg2, diff --git a/linux-user/strace.list b/linux-user/strace.list index 909298099e..4ae60e5e87 100644 --- a/linux-user/strace.list +++ b/linux-user/strace.list @@ -1061,7 +1061,7 @@ { TARGET_NR_prctl, "prctl" , NULL, NULL, NULL }, #endif #ifdef TARGET_NR_pread64 -{ TARGET_NR_pread64, "pread64" , NULL, NULL, NULL }, +{ TARGET_NR_pread64, "pread64" , NULL, print_preadwrite64, NULL }, #endif #ifdef TARGET_NR_preadv { TARGET_NR_preadv, "preadv" , NULL, NULL, NULL }, @@ -1092,7 +1092,7 @@ { TARGET_NR_putpmsg, "putpmsg" , NULL, NULL, NULL }, #endif #ifdef TARGET_NR_pwrite64 -{ TARGET_NR_pwrite64, "pwrite64" , NULL, NULL, NULL }, +{ TARGET_NR_pwrite64, "pwrite64" , NULL, print_preadwrite64, NULL }, #endif #ifdef TARGET_NR_pwritev { TARGET_NR_pwritev, "pwritev" , NULL, NULL, NULL },
Make the strace look nicer for those two syscalls. Signed-off-by: Helge Deller <deller@gmx.de> --- v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier