| Message ID | 20240814233645.944327-4-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | build contrib/plugins using meson | expand |
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
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
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 >
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 >>
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);
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(-)