[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Akihiko Odaki
Subject: Re: [PATCH v3 21/27] plugins: add an API to read registers
Date: Tue, 27 Feb 2024 19:58:43 +0900
User-agent: Mozilla Thunderbird

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


Mostly looks good. I have a few trivial comments so please have a look
at them.
+        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: 
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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]