| Message ID | dbb6dbf1-1ceb-48c0-8174-ee5dea7533dc@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | contrib/plugins does not build on 32-bit host | expand |
Hi Richard, On 12/13/24 13:47, Richard Henderson wrote: > Hi, > > Several of the recent contrib/plugins/ patches do not build on e.g. arm32. > All of the issues are related to casting between pointers and uint64_t; there is a Werror > generated for casting between pointers and integers of different sizes. > > I suspect all of the instances will need to use separate structures to store uint64_t > within the hash tables. The hash values themselves can use uintptr_t, as "hash" by > definition loses data. > > The following is *not* a suggested patch, just touches every place with an error to > highlight all of the places. > This is something I already tried to fix this way, but alas, casting values is not enough, we might lose information (in the case where guest is 64 bits). Some plugins need a refactoring to allocate data dynamically, instead of hiding it under a pointer. See this previous series: https://patchew.org/QEMU/20240814233645.944327-1-pierrick.bouvier@linaro.org/ Finally, we discussed it was not worth the effort, and Alex simply deactivated plugins by default for 32 bits platform, so it should not be built for arm 32 bits. If we really have someone that needs this usecase, we might make the effort, but for now, it does not seem worth the hassle. Note: we already had those warnings before, but since plugins used to be built by an external Makefile, werror was not enabled, so functionally it was already "broken". > > r~ > > > diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c > index 512ef6776b..9f1b05fc35 100644 > --- a/contrib/plugins/cache.c > +++ b/contrib/plugins/cache.c > @@ -474,7 +474,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > uint64_t effective_addr; > > if (sys) { > - effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn); > + effective_addr = (uint64_t)(uintptr_t) qemu_plugin_insn_haddr(insn); > } else { > effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn); > } > diff --git a/contrib/plugins/cflow.c b/contrib/plugins/cflow.c > index b39974d1cf..8f8ebf87cd 100644 > --- a/contrib/plugins/cflow.c > +++ b/contrib/plugins/cflow.c > @@ -215,10 +215,10 @@ static NodeData *fetch_node(uint64_t addr, bool create_if_not_found) > NodeData *node = NULL; > > g_mutex_lock(&node_lock); > - node = (NodeData *) g_hash_table_lookup(nodes, (gconstpointer) addr); > + node = (NodeData *) g_hash_table_lookup(nodes, (gconstpointer)(uintptr_t) addr); > if (!node && create_if_not_found) { > node = create_node(addr); > - g_hash_table_insert(nodes, (gpointer) addr, (gpointer) node); > + g_hash_table_insert(nodes, (gpointer)(uintptr_t) addr, (gpointer) node); > } > g_mutex_unlock(&node_lock); > return node; > diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c > index 02bc5078bd..9b3d356dea 100644 > --- a/contrib/plugins/hotblocks.c > +++ b/contrib/plugins/hotblocks.c > @@ -111,7 +111,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > ExecCount *cnt; > uint64_t pc = qemu_plugin_tb_vaddr(tb); > size_t insns = qemu_plugin_tb_n_insns(tb); > - uint64_t hash = pc ^ insns; > + uintptr_t hash = pc ^ insns; > > g_mutex_lock(&lock); > cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash); > diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c > index 739ac0c66b..6d84ea77f2 100644 > --- a/contrib/plugins/hwprofile.c > +++ b/contrib/plugins/hwprofile.c > @@ -169,7 +169,7 @@ static IOLocationCounts *new_location(GHashTable *table, uint64_t > off_or_pc) > { > IOLocationCounts *loc = g_new0(IOLocationCounts, 1); > loc->off_or_pc = off_or_pc; > - g_hash_table_insert(table, (gpointer) off_or_pc, loc); > + g_hash_table_insert(table, (gpointer)(uintptr_t) off_or_pc, loc); > return loc; > } > > @@ -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,7 @@ 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)(uintptr_t) (source ? qemu_plugin_insn_vaddr(insn) : 0); > qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr, > QEMU_PLUGIN_CB_NO_REGS, > rw, udata); >
On 12/13/24 21:44, Pierrick Bouvier wrote: > Hi Richard, > > On 12/13/24 13:47, Richard Henderson wrote: >> Hi, >> >> Several of the recent contrib/plugins/ patches do not build on e.g. arm32. >> All of the issues are related to casting between pointers and uint64_t; there is a Werror >> generated for casting between pointers and integers of different sizes. >> >> I suspect all of the instances will need to use separate structures to store uint64_t >> within the hash tables. The hash values themselves can use uintptr_t, as "hash" by >> definition loses data. >> >> The following is *not* a suggested patch, just touches every place with an error to >> highlight all of the places. >> > > This is something I already tried to fix this way, but alas, casting values is not enough, > we might lose information (in the case where guest is 64 bits). Some plugins need a > refactoring to allocate data dynamically, instead of hiding it under a pointer. > > See this previous series: > https://patchew.org/QEMU/20240814233645.944327-1-pierrick.bouvier@linaro.org/ > > Finally, we discussed it was not worth the effort, and Alex simply deactivated plugins by > default for 32 bits platform, so it should not be built for arm 32 bits. If we really have > someone that needs this usecase, we might make the effort, but for now, it does not seem > worth the hassle. Hmm. I didn't delete my 32-bit build tree, but it certainly re-configured. If plugins are supposed to be disabled, something may be wrong there... r~
On 12/13/24 21:29, Richard Henderson wrote: > On 12/13/24 21:44, Pierrick Bouvier wrote: >> Hi Richard, >> >> On 12/13/24 13:47, Richard Henderson wrote: >>> Hi, >>> >>> Several of the recent contrib/plugins/ patches do not build on e.g. arm32. >>> All of the issues are related to casting between pointers and uint64_t; there is a Werror >>> generated for casting between pointers and integers of different sizes. >>> >>> I suspect all of the instances will need to use separate structures to store uint64_t >>> within the hash tables. The hash values themselves can use uintptr_t, as "hash" by >>> definition loses data. >>> >>> The following is *not* a suggested patch, just touches every place with an error to >>> highlight all of the places. >>> >> >> This is something I already tried to fix this way, but alas, casting values is not enough, >> we might lose information (in the case where guest is 64 bits). Some plugins need a >> refactoring to allocate data dynamically, instead of hiding it under a pointer. >> >> See this previous series: >> https://patchew.org/QEMU/20240814233645.944327-1-pierrick.bouvier@linaro.org/ >> >> Finally, we discussed it was not worth the effort, and Alex simply deactivated plugins by >> default for 32 bits platform, so it should not be built for arm 32 bits. If we really have >> someone that needs this usecase, we might make the effort, but for now, it does not seem >> worth the hassle. > > Hmm. I didn't delete my 32-bit build tree, but it certainly re-configured. If plugins > are supposed to be disabled, something may be wrong there... > The 32 bits check is done in the configure script, not in meson part. From a clean source tree, on a armhf machine, it is deactivated as expected. > > r~
On 14/12/24 06:29, Richard Henderson wrote: > On 12/13/24 21:44, Pierrick Bouvier wrote: >> Hi Richard, >> >> On 12/13/24 13:47, Richard Henderson wrote: >>> Hi, >>> >>> Several of the recent contrib/plugins/ patches do not build on e.g. >>> arm32. >>> All of the issues are related to casting between pointers and >>> uint64_t; there is a Werror >>> generated for casting between pointers and integers of different sizes. >>> >>> I suspect all of the instances will need to use separate structures >>> to store uint64_t >>> within the hash tables. The hash values themselves can use >>> uintptr_t, as "hash" by >>> definition loses data. >>> >>> The following is *not* a suggested patch, just touches every place >>> with an error to >>> highlight all of the places. >>> >> >> This is something I already tried to fix this way, but alas, casting >> values is not enough, we might lose information (in the case where >> guest is 64 bits). Some plugins need a refactoring to allocate data >> dynamically, instead of hiding it under a pointer. >> >> See this previous series: >> https://patchew.org/QEMU/20240814233645.944327-1- >> pierrick.bouvier@linaro.org/ >> >> Finally, we discussed it was not worth the effort, and Alex simply >> deactivated plugins by default for 32 bits platform, so it should not >> be built for arm 32 bits. If we really have someone that needs this >> usecase, we might make the effort, but for now, it does not seem worth >> the hassle. This is: commit cf2a78cbbb463d5716da9805c8fc5758937924d8 Author: Alex Bennée <alex.bennee@linaro.org> Date: Mon Sep 16 09:53:43 2024 +0100 deprecation: don't enable TCG plugins by default on 32 bit hosts The existing plugins already liberally use host pointer stuffing for passing user data which will fail when doing 64 bit guests on 32 bit hosts. We should discourage this by officially deprecating support and adding another nail to the 32 bit host coffin. ... +TCG Plugin support not enabled by default on 32-bit hosts (since 9.2) +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' ... -if test "$plugins" != "no"; then +if test "$plugins" != "no" && test $host_bits -eq 64; then plugins=yes > Hmm. I didn't delete my 32-bit build tree, but it certainly re- > configured. If plugins are supposed to be disabled, something may be > wrong there... > > > r~ >
On 12/14/24 04:35, Philippe Mathieu-Daudé wrote: > On 14/12/24 06:29, Richard Henderson wrote: >> On 12/13/24 21:44, Pierrick Bouvier wrote: >>> Hi Richard, >>> >>> On 12/13/24 13:47, Richard Henderson wrote: >>>> Hi, >>>> >>>> Several of the recent contrib/plugins/ patches do not build on e.g. >>>> arm32. >>>> All of the issues are related to casting between pointers and >>>> uint64_t; there is a Werror >>>> generated for casting between pointers and integers of different sizes. >>>> >>>> I suspect all of the instances will need to use separate structures >>>> to store uint64_t >>>> within the hash tables. The hash values themselves can use >>>> uintptr_t, as "hash" by >>>> definition loses data. >>>> >>>> The following is *not* a suggested patch, just touches every place >>>> with an error to >>>> highlight all of the places. >>>> >>> >>> This is something I already tried to fix this way, but alas, casting >>> values is not enough, we might lose information (in the case where >>> guest is 64 bits). Some plugins need a refactoring to allocate data >>> dynamically, instead of hiding it under a pointer. >>> >>> See this previous series: >>> https://patchew.org/QEMU/20240814233645.944327-1- >>> pierrick.bouvier@linaro.org/ >>> >>> Finally, we discussed it was not worth the effort, and Alex simply >>> deactivated plugins by default for 32 bits platform, so it should not >>> be built for arm 32 bits. If we really have someone that needs this >>> usecase, we might make the effort, but for now, it does not seem worth >>> the hassle. > > This is: > > commit cf2a78cbbb463d5716da9805c8fc5758937924d8 > Author: Alex Bennée <alex.bennee@linaro.org> > Date: Mon Sep 16 09:53:43 2024 +0100 > > deprecation: don't enable TCG plugins by default on 32 bit hosts > > The existing plugins already liberally use host pointer stuffing for > passing user data which will fail when doing 64 bit guests on 32 bit > hosts. We should discourage this by officially deprecating support and > adding another nail to the 32 bit host coffin. > > ... > > +TCG Plugin support not enabled by default on 32-bit hosts (since 9.2) > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > > ... > > -if test "$plugins" != "no"; then > +if test "$plugins" != "no" && test $host_bits -eq 64; then > plugins=yes > >> Hmm. I didn't delete my 32-bit build tree, but it certainly re- >> configured. If plugins are supposed to be disabled, something may be >> wrong there... >> To add more details, most of the problems with plugins comes from converting uint64 to pointers, for use in hash tables. The macros GINT_TO_POINTER and GUINT_TO_POINTER are casting to long before casting to pointer, so they inhibit the conversion warning we should normally have. IMHO, it's a bad thing and very error prone even though the Glib documentation mentions this. In short, the best way to deal with this would be get rid of GUINT_TO_POINTER and GINT_TO_POINTER in plugins code, and fix all the warnings related. >> >> r~ >> >
Richard Henderson <richard.henderson@linaro.org> writes: > On 12/13/24 21:44, Pierrick Bouvier wrote: >> Hi Richard, >> On 12/13/24 13:47, Richard Henderson wrote: >>> Hi, >>> >>> Several of the recent contrib/plugins/ patches do not build on e.g. arm32. >>> All of the issues are related to casting between pointers and uint64_t; there is a Werror >>> generated for casting between pointers and integers of different sizes. >>> >>> I suspect all of the instances will need to use separate structures to store uint64_t >>> within the hash tables. The hash values themselves can use uintptr_t, as "hash" by >>> definition loses data. >>> >>> The following is *not* a suggested patch, just touches every place with an error to >>> highlight all of the places. >>> >> This is something I already tried to fix this way, but alas, casting >> values is not enough, we might lose information (in the case where >> guest is 64 bits). Some plugins need a refactoring to allocate data >> dynamically, instead of hiding it under a pointer. >> See this previous series: >> https://patchew.org/QEMU/20240814233645.944327-1-pierrick.bouvier@linaro.org/ >> Finally, we discussed it was not worth the effort, and Alex simply >> deactivated plugins by default for 32 bits platform, so it should >> not be built for arm 32 bits. If we really have someone that needs >> this usecase, we might make the effort, but for now, it does not >> seem worth the hassle. > > Hmm. I didn't delete my 32-bit build tree, but it certainly > re-configured. If plugins are supposed to be disabled, something may > be wrong there... Something should have triggered re-running config.status and triggering the disable. I wonder why that didn't work.
On 12/16/24 10:45, Pierrick Bouvier wrote: > On 12/14/24 04:35, Philippe Mathieu-Daudé wrote: >> On 14/12/24 06:29, Richard Henderson wrote: >>> On 12/13/24 21:44, Pierrick Bouvier wrote: >>>> Hi Richard, >>>> >>>> On 12/13/24 13:47, Richard Henderson wrote: >>>>> Hi, >>>>> >>>>> Several of the recent contrib/plugins/ patches do not build on e.g. >>>>> arm32. >>>>> All of the issues are related to casting between pointers and >>>>> uint64_t; there is a Werror >>>>> generated for casting between pointers and integers of different sizes. >>>>> >>>>> I suspect all of the instances will need to use separate structures >>>>> to store uint64_t >>>>> within the hash tables. The hash values themselves can use >>>>> uintptr_t, as "hash" by >>>>> definition loses data. >>>>> >>>>> The following is *not* a suggested patch, just touches every place >>>>> with an error to >>>>> highlight all of the places. >>>>> >>>> >>>> This is something I already tried to fix this way, but alas, casting >>>> values is not enough, we might lose information (in the case where >>>> guest is 64 bits). Some plugins need a refactoring to allocate data >>>> dynamically, instead of hiding it under a pointer. >>>> >>>> See this previous series: >>>> https://patchew.org/QEMU/20240814233645.944327-1- >>>> pierrick.bouvier@linaro.org/ >>>> >>>> Finally, we discussed it was not worth the effort, and Alex simply >>>> deactivated plugins by default for 32 bits platform, so it should not >>>> be built for arm 32 bits. If we really have someone that needs this >>>> usecase, we might make the effort, but for now, it does not seem worth >>>> the hassle. >> >> This is: >> >> commit cf2a78cbbb463d5716da9805c8fc5758937924d8 >> Author: Alex Bennée <alex.bennee@linaro.org> >> Date: Mon Sep 16 09:53:43 2024 +0100 >> >> deprecation: don't enable TCG plugins by default on 32 bit hosts >> >> The existing plugins already liberally use host pointer stuffing for >> passing user data which will fail when doing 64 bit guests on 32 bit >> hosts. We should discourage this by officially deprecating support and >> adding another nail to the 32 bit host coffin. >> >> ... >> >> +TCG Plugin support not enabled by default on 32-bit hosts (since 9.2) >> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' >> >> ... >> >> -if test "$plugins" != "no"; then >> +if test "$plugins" != "no" && test $host_bits -eq 64; then >> plugins=yes >> >>> Hmm. I didn't delete my 32-bit build tree, but it certainly re- >>> configured. If plugins are supposed to be disabled, something may be >>> wrong there... >>> > > To add more details, most of the problems with plugins comes from > converting uint64 to pointers, for use in hash tables. > > The macros GINT_TO_POINTER and GUINT_TO_POINTER are casting to long > before casting to pointer, so they inhibit the conversion warning we > should normally have. IMHO, it's a bad thing and very error prone even > though the Glib documentation mentions this. > > In short, the best way to deal with this would be get rid of > GUINT_TO_POINTER and GINT_TO_POINTER in plugins code, and fix all the > warnings related. > I will push a series to fix 32 bits builds for plugins (all done, I'm now testing it to ensure it does not break modified plugins). I was a bit hesitant to do it before, but now that we build contrib plugins with meson, it makes sense to support it. In more, the change is pretty straightforward. I'll revert the configure change to reenable plugins by default as well. >>> >>> r~ >>> >> >
On 12/13/24 13:47, Richard Henderson wrote: > Hi, > > Several of the recent contrib/plugins/ patches do not build on e.g. arm32. > All of the issues are related to casting between pointers and uint64_t; there is a Werror > generated for casting between pointers and integers of different sizes. > > I suspect all of the instances will need to use separate structures to store uint64_t > within the hash tables. The hash values themselves can use uintptr_t, as "hash" by > definition loses data. > > The following is *not* a suggested patch, just touches every place with an error to > highlight all of the places. > > > r~ > > > diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c > index 512ef6776b..9f1b05fc35 100644 > --- a/contrib/plugins/cache.c > +++ b/contrib/plugins/cache.c > @@ -474,7 +474,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > uint64_t effective_addr; > > if (sys) { > - effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn); > + effective_addr = (uint64_t)(uintptr_t) qemu_plugin_insn_haddr(insn); > } else { > effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn); > } > diff --git a/contrib/plugins/cflow.c b/contrib/plugins/cflow.c > index b39974d1cf..8f8ebf87cd 100644 > --- a/contrib/plugins/cflow.c > +++ b/contrib/plugins/cflow.c > @@ -215,10 +215,10 @@ static NodeData *fetch_node(uint64_t addr, bool create_if_not_found) > NodeData *node = NULL; > > g_mutex_lock(&node_lock); > - node = (NodeData *) g_hash_table_lookup(nodes, (gconstpointer) addr); > + node = (NodeData *) g_hash_table_lookup(nodes, (gconstpointer)(uintptr_t) addr); > if (!node && create_if_not_found) { > node = create_node(addr); > - g_hash_table_insert(nodes, (gpointer) addr, (gpointer) node); > + g_hash_table_insert(nodes, (gpointer)(uintptr_t) addr, (gpointer) node); > } > g_mutex_unlock(&node_lock); > return node; > diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c > index 02bc5078bd..9b3d356dea 100644 > --- a/contrib/plugins/hotblocks.c > +++ b/contrib/plugins/hotblocks.c > @@ -111,7 +111,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > ExecCount *cnt; > uint64_t pc = qemu_plugin_tb_vaddr(tb); > size_t insns = qemu_plugin_tb_n_insns(tb); > - uint64_t hash = pc ^ insns; > + uintptr_t hash = pc ^ insns; > > g_mutex_lock(&lock); > cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash); > diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c > index 739ac0c66b..6d84ea77f2 100644 > --- a/contrib/plugins/hwprofile.c > +++ b/contrib/plugins/hwprofile.c > @@ -169,7 +169,7 @@ static IOLocationCounts *new_location(GHashTable *table, uint64_t > off_or_pc) > { > IOLocationCounts *loc = g_new0(IOLocationCounts, 1); > loc->off_or_pc = off_or_pc; > - g_hash_table_insert(table, (gpointer) off_or_pc, loc); > + g_hash_table_insert(table, (gpointer)(uintptr_t) off_or_pc, loc); > return loc; > } > > @@ -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,7 @@ 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)(uintptr_t) (source ? qemu_plugin_insn_vaddr(insn) : 0); > qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr, > QEMU_PLUGIN_CB_NO_REGS, > rw, udata); > The series fixing this was sent: https://lore.kernel.org/qemu-devel/20241217010707.2557258-1-pierrick.bouvier@linaro.org/T/#t Pierrick
diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c index 512ef6776b..9f1b05fc35 100644 --- a/contrib/plugins/cache.c +++ b/contrib/plugins/cache.c @@ -474,7 +474,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) uint64_t effective_addr; if (sys) { - effective_addr = (uint64_t) qemu_plugin_insn_haddr(insn); + effective_addr = (uint64_t)(uintptr_t) qemu_plugin_insn_haddr(insn); } else { effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn); } diff --git a/contrib/plugins/cflow.c b/contrib/plugins/cflow.c index b39974d1cf..8f8ebf87cd 100644 --- a/contrib/plugins/cflow.c +++ b/contrib/plugins/cflow.c @@ -215,10 +215,10 @@ static NodeData *fetch_node(uint64_t addr, bool create_if_not_found) NodeData *node = NULL; g_mutex_lock(&node_lock); - node = (NodeData *) g_hash_table_lookup(nodes, (gconstpointer) addr); + node = (NodeData *) g_hash_table_lookup(nodes, (gconstpointer)(uintptr_t) addr); if (!node && create_if_not_found) { node = create_node(addr); - g_hash_table_insert(nodes, (gpointer) addr, (gpointer) node); + g_hash_table_insert(nodes, (gpointer)(uintptr_t) addr, (gpointer) node); } g_mutex_unlock(&node_lock); return node; diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c index 02bc5078bd..9b3d356dea 100644 --- a/contrib/plugins/hotblocks.c +++ b/contrib/plugins/hotblocks.c @@ -111,7 +111,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) ExecCount *cnt; uint64_t pc = qemu_plugin_tb_vaddr(tb); size_t insns = qemu_plugin_tb_n_insns(tb); - uint64_t hash = pc ^ insns; + uintptr_t hash = pc ^ insns; g_mutex_lock(&lock); cnt = (ExecCount *) g_hash_table_lookup(hotblocks, (gconstpointer) hash); diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c index 739ac0c66b..6d84ea77f2 100644 --- a/contrib/plugins/hwprofile.c +++ b/contrib/plugins/hwprofile.c @@ -169,7 +169,7 @@ static IOLocationCounts *new_location(GHashTable *table, uint64_t off_or_pc) { IOLocationCounts *loc = g_new0(IOLocationCounts, 1); loc->off_or_pc = off_or_pc; - g_hash_table_insert(table, (gpointer) off_or_pc, loc); + g_hash_table_insert(table, (gpointer)(uintptr_t) off_or_pc, loc); return loc; }
Hi, Several of the recent contrib/plugins/ patches do not build on e.g. arm32. All of the issues are related to casting between pointers and uint64_t; there is a Werror generated for casting between pointers and integers of different sizes. I suspect all of the instances will need to use separate structures to store uint64_t within the hash tables. The hash values themselves can use uintptr_t, as "hash" by definition loses data. The following is *not* a suggested patch, just touches every place with an error to highlight all of the places. r~ @@ -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,7 @@ 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)(uintptr_t) (source ? qemu_plugin_insn_vaddr(insn) : 0); qemu_plugin_register_vcpu_mem_cb(insn, vcpu_haddr, QEMU_PLUGIN_CB_NO_REGS, rw, udata);