1. Patchew
  2. QEMU
  3. [PATCH 0/6] build contrib/plugins using meson
  4. View patch

[PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host

Pierrick Bouvier posted 6 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host
Posted by Pierrick Bouvier 3 months, 1 week ago 
Found on debian stable (i386).

../contrib/plugins/hwprofile.c: In function 'new_location':
../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
      |                                ^
../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  227 |             off = (uint64_t) udata;
      |                   ^
../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  232 |                                                              (gpointer) off);
      |                                                              ^
../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
      |

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 contrib/plugins/hwprofile.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c
index 739ac0c66b5..ee94a74ad94 100644
--- a/contrib/plugins/hwprofile.c
+++ b/contrib/plugins/hwprofile.c
@@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
     return count;
 }
 
-static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc)
+static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
 {
     IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
     loc->off_or_pc = off_or_pc;
@@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
         return;
     } else {
         const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
-        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
+        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
         bool is_write = qemu_plugin_mem_is_store(meminfo);
         DeviceCounts *counts;
 
@@ -224,7 +224,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
 
         /* either track offsets or source of access */
         if (source) {
-            off = (uint64_t) udata;
+            off = (uintptr_t) udata;
         }
 
         if (pattern || source) {
@@ -247,7 +247,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 
     for (i = 0; i < n; i++) {
         struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
-        gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
+        gpointer udata = (gpointer) (
+                source ? (uintptr_t) qemu_plugin_insn_vaddr(insn) : 0);
         qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr,
                                          QEMU_PLUGIN_CB_NO_REGS,
                                          rw, udata);
-- 
2.39.2
Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host
Posted by Thomas Huth 3 months, 1 week ago 
On 15/08/2024 01.36, Pierrick Bouvier wrote:
> Found on debian stable (i386).
> 
> ../contrib/plugins/hwprofile.c: In function 'new_location':
> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>        |                                ^
> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>    227 |             off = (uint64_t) udata;
>        |                   ^
> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    232 |                                                              (gpointer) off);
>        |                                                              ^
> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>        |
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   contrib/plugins/hwprofile.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c
> index 739ac0c66b5..ee94a74ad94 100644
> --- a/contrib/plugins/hwprofile.c
> +++ b/contrib/plugins/hwprofile.c
> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>       return count;
>   }
>   
> -static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc)
> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>   {
>       IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>       loc->off_or_pc = off_or_pc;
> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>           return;
>       } else {
>           const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);

qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks wrong to me.

  Thomas
Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host
Posted by Alex Bennée 3 months, 1 week ago 
Thomas Huth <thuth@redhat.com> writes:

> On 15/08/2024 01.36, Pierrick Bouvier wrote:
>> Found on debian stable (i386).
>> ../contrib/plugins/hwprofile.c: In function 'new_location':
>> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>    172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>>        |                                ^
>> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
>> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>    227 |             off = (uint64_t) udata;
>>        |                   ^
>> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>    232 |                                                              (gpointer) off);
>>        |                                                              ^
>> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
>> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>    250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>>        |
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   contrib/plugins/hwprofile.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>> diff --git a/contrib/plugins/hwprofile.c
>> b/contrib/plugins/hwprofile.c
>> index 739ac0c66b5..ee94a74ad94 100644
>> --- a/contrib/plugins/hwprofile.c
>> +++ b/contrib/plugins/hwprofile.c
>> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>>       return count;
>>   }
>>   -static IOLocationCounts *new_location(GHashTable *table, uint64_t
>> off_or_pc)
>> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>>   {
>>       IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>>       loc->off_or_pc = off_or_pc;
>> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>>           return;
>>       } else {
>>           const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>
> qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks
> wrong to me.

It is. However it just goes to show you should be expecting to
instrument 64 bit code with a 32 bit host because you can't do pointer
stuffing tricks like this.

Maybe we could just disable plugins on 32 bit hosts?

>
>  Thomas

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host
Posted by Pierrick Bouvier 3 months, 1 week ago 
On 8/15/24 05:03, 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/hwprofile.c: In function 'new_location':
>>> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>     172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>>>         |                                ^
>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
>>> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>     227 |             off = (uint64_t) udata;
>>>         |                   ^
>>> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>     232 |                                                              (gpointer) off);
>>>         |                                                              ^
>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
>>> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>     250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>>>         |
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    contrib/plugins/hwprofile.c | 9 +++++----
>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>> diff --git a/contrib/plugins/hwprofile.c
>>> b/contrib/plugins/hwprofile.c
>>> index 739ac0c66b5..ee94a74ad94 100644
>>> --- a/contrib/plugins/hwprofile.c
>>> +++ b/contrib/plugins/hwprofile.c
>>> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>>>        return count;
>>>    }
>>>    -static IOLocationCounts *new_location(GHashTable *table, uint64_t
>>> off_or_pc)
>>> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>>>    {
>>>        IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>>>        loc->off_or_pc = off_or_pc;
>>> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>>>            return;
>>>        } else {
>>>            const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>>> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>
>> qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks
>> wrong to me.
> 
> It is. However it just goes to show you should be expecting to
> instrument 64 bit code with a 32 bit host because you can't do pointer
> stuffing tricks like this.
> 
> Maybe we could just disable plugins on 32 bit hosts?
> 

Only two plugins are concerned by this problem, it's worth fixing them 
correctly (i.e. not use 64 bits data directly as a pointer, but allocate 
memory and pass pointer instead).

>>
>>   Thomas
>
Re: [PATCH 3/6] contrib/plugins/hwprofile: fix warning when compiling on 32bits host
Posted by Pierrick Bouvier 3 months, 1 week ago 
On 8/15/24 10:40, Pierrick Bouvier wrote:
> On 8/15/24 05:03, 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/hwprofile.c: In function 'new_location':
>>>> ../contrib/plugins/hwprofile.c:172:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>      172 |     g_hash_table_insert(table, (gpointer) off_or_pc, loc);
>>>>          |                                ^
>>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_haddr':
>>>> ../contrib/plugins/hwprofile.c:227:19: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
>>>>      227 |             off = (uint64_t) udata;
>>>>          |                   ^
>>>> ../contrib/plugins/hwprofile.c:232:62: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>      232 |                                                              (gpointer) off);
>>>>          |                                                              ^
>>>> ../contrib/plugins/hwprofile.c: In function 'vcpu_tb_trans':
>>>> ../contrib/plugins/hwprofile.c:250:26: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>      250 |         gpointer udata = (gpointer) (source ? qemu_plugin_insn_vaddr(insn) : 0);
>>>>          |
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     contrib/plugins/hwprofile.c | 9 +++++----
>>>>     1 file changed, 5 insertions(+), 4 deletions(-)
>>>> diff --git a/contrib/plugins/hwprofile.c
>>>> b/contrib/plugins/hwprofile.c
>>>> index 739ac0c66b5..ee94a74ad94 100644
>>>> --- a/contrib/plugins/hwprofile.c
>>>> +++ b/contrib/plugins/hwprofile.c
>>>> @@ -165,7 +165,7 @@ static DeviceCounts *new_count(const char *name, uint64_t base)
>>>>         return count;
>>>>     }
>>>>     -static IOLocationCounts *new_location(GHashTable *table, uint64_t
>>>> off_or_pc)
>>>> +static IOLocationCounts *new_location(GHashTable *table, uintptr_t off_or_pc)
>>>>     {
>>>>         IOLocationCounts *loc = g_new0(IOLocationCounts, 1);
>>>>         loc->off_or_pc = off_or_pc;
>>>> @@ -201,7 +201,7 @@ static void vcpu_haddr(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
>>>>             return;
>>>>         } else {
>>>>             const char *name = qemu_plugin_hwaddr_device_name(hwaddr);
>>>> -        uint64_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>>> +        uintptr_t off = qemu_plugin_hwaddr_phys_addr(hwaddr);
>>>
>>> qemu_plugin_hwaddr_phys_addr() returns an uint64_t, so this looks
>>> wrong to me.
>>
>> It is. However it just goes to show you should be expecting to
>> instrument 64 bit code with a 32 bit host because you can't do pointer
>> stuffing tricks like this.
>>
>> Maybe we could just disable plugins on 32 bit hosts?
>>
> 
> Only two plugins are concerned by this problem, it's worth fixing them
> correctly (i.e. not use 64 bits data directly as a pointer, but allocate
> memory and pass pointer instead).
> 

After looking more closely at existing plugins, the problem is wider 
than expected (and build warnings).
The GLib macro (GUINT_TO_POINTER) silently transform an int64 value to a 
pointer, including on 32 bits platform. It performs a double case 
(unsigned long -> pointer).

Thus, even if there is no warning, some plugins are broken because of 
data (either because they pass data in callback, or they have an address 
based hashtable).

It could be fixable, but the wiser solution is probably to disable 
plugins on 32 bits platform, like Alex proposed.

>>>
>>>    Thomas
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.