[PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Helge Deller]

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));
+    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 },

Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Richard Henderson]

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~

Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Laurent Vivier]

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

Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Helge Deller]

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

Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Laurent Vivier]

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

Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Helge Deller]

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

Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Laurent Vivier]

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
>