Re: [PATCH v3 21/27] plugins: add an API to read registers

[Akihiko Odaki]

On 2024/02/27 19:29, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
> > On 2024/02/27 19:08, Alex Bennée wrote:
> > > Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> > >
> > > > On 2024/02/27 1:56, Alex Bennée wrote:
> > > > > We can only request a list of registers once the vCPU has been
> > > > > initialised so the user needs to use either call the get function on
> > > > > vCPU initialisation or during the translation phase.
> > > > > We don't expose the reg number to the plugin instead hiding it
> > > > > behind
> > > > > an opaque handle. For now this is just the gdb_regnum encapsulated in
> > > > > an anonymous GPOINTER but in future as we add more state for plugins
> > > > > to track we can expand it.
> > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706
> > > > > Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > > Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org>
> > > > > Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com>
> > > > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > > > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > > >
> > > > Hi,
> > > >
> > > > Mostly looks good. I have a few trivial comments so please have a look
> > > > at them.
> > > Done
> > > <snip>
> > > > > +        g_array_append_val(find_data, desc);
> > > > > +    }
> > > > > +
> > > > > +    return find_data;
> > > > > +}
> > > > > +
> > > > > +GArray *qemu_plugin_get_registers(void)
> > > > > +{
> > > > > +    g_assert(current_cpu);
> > > > > +
> > > > > +    g_autoptr(GArray) regs = gdb_get_register_list(current_cpu);
> > > > > +    return regs->len ? create_register_handles(current_cpu, regs) : NULL;
> > > >
> > > > Why do you need regs->len check?
> > > Not all guests expose register to gdb so we need to catch that:
> > >     TEST    catch-syscalls-with-libinsn.so on alpha
> > > **
> > > ERROR:../../plugins/api.c:459:qemu_plugin_get_registers: assertion failed: 
> > > (regs->len)
> > > Aborted
> > Certainly regs->len can be 0, but why do you need to return NULL in
> > that case? Can't qemu_plugin_get_registers() return an empty array
> > just as gdb_get_register_list() does?
>
> That seems more troublesome to handle the other end. NULL is nothing is
> a fairly common pattern here which makes short-circuiting the array
> iteration simple.

I'm not sure why you would want to short-circuiting array iteration. 
Iterating an empty array is often simpler.

In any case, the same logic also applies to gdb_get_register_list(), so 
you should change gdb_get_register_list() to return NULL instead of an 
empty array if you think that's more reasonable.