Re: [PATCH v2 5/6] hw/input/stellaris_input: Convert to qdev

[Philippe Mathieu-Daudé]

On 30/10/23 15:41, Peter Maydell wrote:
> On Mon, 30 Oct 2023 at 13:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Hi Peter,
> >
> > Cc'ing Markus for QObject.
> >
> > On 30/10/23 12:48, Peter Maydell wrote:
> > > Convert the hw/input/stellaris_input device to qdev.
> > >
> > > The interface uses an array property for the board to specify the
> > > keycodes to use, so the s->keycodes memory is now allocated by the
> > > array-property machinery.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > > v1->v2: drop private/public comment lines
> > > ---
> > >    include/hw/input/stellaris_gamepad.h | 22 ++++++++-
> > >    hw/arm/stellaris.c                   | 26 +++++++---
> > >    hw/input/stellaris_gamepad.c         | 73 +++++++++++++++++++---------
> > >    3 files changed, 89 insertions(+), 32 deletions(-)
> >
> >
> > > diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> > > index 96585dd7106..707b0dae375 100644
> > > --- a/hw/arm/stellaris.c
> > > +++ b/hw/arm/stellaris.c
> > > @@ -31,6 +31,7 @@
> > >    #include "hw/timer/stellaris-gptm.h"
> > >    #include "hw/qdev-clock.h"
> > >    #include "qom/object.h"
> > > +#include "qapi/qmp/qlist.h"
> > >
> > >    #define GPIO_A 0
> > >    #define GPIO_B 1
> > > @@ -1274,16 +1275,27 @@ static void stellaris_init(MachineState *ms, 
> > > stellaris_board_info *board)
> > >            sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic, 
> > > 42));
> > >        }
> > >        if (board->peripherals & BP_GAMEPAD) {
> > > -        qemu_irq gpad_irq[5];
> > > +        QList *gpad_keycode_list = qlist_new();
> >
> > I'm trying to understand better qlist_new(), but unfortunately there
> > is not much documentation. Looking at how the allocated list was
> > released, I found use of g_autoptr in tests/unit/check-qobject.c,
> > so I tried:
> >
> >              g_autoptr(QList) gpad_keycode_list = qlist_new();
>
> The API for qdev_prop_set_array() documents that it takes ownership
> of the list you pass it (and it ends up calling qobject_unref() on it).
> So I think adding g_autoptr() here will result in the memory being
> double-freed (once inside qobject_unref() when the refcount
> goes to 0, and once when g_autoptr frees it as it goes out of scope)...

Ah, I missed how qdev_prop_set_array() is involved.

> > * thread #2, stop reason = signal SIGABRT
> >     * frame #0: 0x8b1eb11c libsystem_kernel.dylib`__pthread_kill + 8
> >       frame #1: 0x8b222cc0 libsystem_pthread.dylib`pthread_kill + 288
> >       frame #2: 0x8b132a50 libsystem_c.dylib`abort + 180
> >       frame #3: 0x8b049b08 libsystem_malloc.dylib`malloc_vreport + 908
> >       frame #4: 0x8b06924c libsystem_malloc.dylib`malloc_zone_error + 104
> >       frame #5: 0x8b05b094
> > libsystem_malloc.dylib`nanov2_guard_corruption_detected + 44
> >       frame #6: 0x8b05a2a8
> > libsystem_malloc.dylib`nanov2_allocate_outlined + 404
> >       frame #7: 0x0201fdc0 libglib-2.0.0.dylib`g_malloc0 + 36
> >       frame #8: 0x02007718 libglib-2.0.0.dylib`g_hash_table_setup_storage
> > + 76
> >       frame #9: 0x020076b0 libglib-2.0.0.dylib`g_hash_table_new_full + 96
>
> ...which is probably why a later memory operation runs into a
> malloc data corruption assertion.

Yes, this is certainly the reason. Thanks for the explanation!

Phil.