Re: contrib/plugins does not build on 32-bit host

[Pierrick Bouvier]

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);