contrib/plugins does not build on 32-bit host

contrib/plugins does not build on 32-bit host

[Richard Henderson]

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

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

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

[Richard Henderson]

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~

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

[Pierrick Bouvier]

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~

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

[Philippe Mathieu-Daudé]

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~
> 

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

[Pierrick Bouvier]

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~
>>
> 

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

[Pierrick Bouvier]

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~
>>>
>>
> 

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

[Alex Bennée]

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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Pierrick Bouvier]

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