Re: [PATCH 2/6] contrib/plugins/cache: fix warning when compiling on 32bits host

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 8/15/24 04:46, Alex Bennée wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>>> Found on debian stable (i386).
>>> ../contrib/plugins/cache.c: In function 'vcpu_tb_trans':
>>> ../contrib/plugins/cache.c:477:30: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>     477 |             effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>>         |
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    contrib/plugins/cache.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>>> index 512ef6776b7..82ed734d6d4 100644
>>> --- a/contrib/plugins/cache.c
>>> +++ b/contrib/plugins/cache.c
>>> @@ -471,12 +471,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>>>        n_insns = qemu_plugin_tb_n_insns(tb);
>>>        for (i = 0; i < n_insns; i++) {
>>>            struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
>>> -        uint64_t effective_addr;
>>> +        uintptr_t effective_addr;
>>>              if (sys) {
>>> -            effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn);
>>> +            effective_addr = (uintptr_t) qemu_plugin_insn_haddr(insn);
>>>            } else {
>>> -            effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
>>> +            effective_addr = (uintptr_t)
>>> qemu_plugin_insn_vaddr(insn);
>>>            }
>>
>> Is this the right fix? I assume effective_addr stores an address of
>> the guest, so if the guest is 64-bit and the host is 32-bit, you now
>> lose the upper bits of the address...?
> 
> I think the problem is higher up, it was a mistake to have:
> 
>    void *qemu_plugin_insn_haddr(const struct qemu_plugin_insn *insn);
> 
> return *void, at least vaddr returns an explicit 64 bit value which can
> hold everything (at a slight expense to 32bit emulation hosts, but
> seriously stop doing that we've been in the 64bit world for some time
> now).
> 

It's an open question I had. When executing 64 bits binaries on a 32 
bits host, are we emulating the full 64 bits address space, or do we 
restrict to 32 bits? For user mode, I don't see how it could be possible 
to have address space beyond the 32 bits range, but system mode is 
probably different.

The real proper fix is to not encode directly value under udata for 
callbacks, but allocate this and pass a pointer instead.

>>
>> The casting for qemu_plugin_insn_vaddr is not required at all since it
>> already returns an uint64_t, so you can remoe that one. For the haddr
>> part, maybe do a double-cast:
>>
>>    effective_addr = (uint64_t)(uintptr_t)qemu_plugin_insn_haddr(insn)
>>
>> ?
>>
>>   Thomas
>