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~