Re: [RFC PATCH] plugins/api: expose symbol lookup to plugins

["Alex Bennée" <alex.bennee@linaro.org>]

Mahmoud Mandour <ma.mandourr@gmail.com> writes:

> On Tue, Jun 1, 2021 at 4:58 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  This is a quality of life helper for plugins so they don't need to
>  re-implement symbol lookup when dumping an address. The strings are
>  constant so don't need to be duplicated. One minor tweak is to return
>  NULL instead of a zero length string to show lookup failed.
>
> Thank you for implementing this, I found it a really easy addition since you
> already told me how this is done in the kick-off meeting and I implemented it
> but I'm glad you already posted it :D
>
>  Based-on: 20210530063712.6832-4-ma.mandourr@gmail.com
>  Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>  ---
>   include/qemu/qemu-plugin.h |  9 +++++++++
>   contrib/plugins/cache.c    | 10 ++++++++--
>   plugins/api.c              |  6 ++++++
>   3 files changed, 23 insertions(+), 2 deletions(-)
>
>  diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>  index 97cdfd7761..dc3496f36c 100644
>  --- a/include/qemu/qemu-plugin.h
>  +++ b/include/qemu/qemu-plugin.h
>  @@ -525,6 +525,15 @@ qemu_plugin_register_vcpu_syscall_ret_cb(qemu_plugin_id_t id,
>
>   char *qemu_plugin_insn_disas(const struct qemu_plugin_insn *insn);
>
>  +/**
>  + * qemu_plugin_insn_symbol() - best effort symbol lookup
>  + * @insn: instruction reference
>  + *
>  + * Return a static string referring to the symbol. This is dependent
>  + * on the binary QEMU is running having provided a symbol table.
>  + */
>  +const char *qemu_plugin_insn_symbol(const struct qemu_plugin_insn *insn);
>  +
>   /**
>    * qemu_plugin_vcpu_for_each() - iterate over the existing vCPU
>    * @id: plugin ID
>  diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
>  index 1e323494bf..afaa3d9db5 100644
>  --- a/contrib/plugins/cache.c
>  +++ b/contrib/plugins/cache.c
>  @@ -46,6 +46,7 @@ enum AccessResult {
>
>   struct InsnData {
>       char *disas_str;
>  +    const char *symbol;
>       uint64_t addr;
>       uint64_t misses;
>   };
>  @@ -377,10 +378,12 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>           struct InsnData *idata = g_new(struct InsnData, 1);
>
>           ddata->disas_str = qemu_plugin_insn_disas(insn);
>  +        ddata->symbol = qemu_plugin_insn_symbol(insn);
>           ddata->misses = 0;
>           ddata->addr = effective_addr;
>
>           idata->disas_str = g_strdup(ddata->disas_str);
>  +        idata->symbol = qemu_plugin_insn_symbol(insn);
>           idata->misses = 0;
>           idata->addr = effective_addr;
>
>  @@ -397,8 +400,11 @@ static void print_entry(gpointer data)
>   {
>       struct InsnData *insn = (struct InsnData *) data;
>       g_autoptr(GString) xx = g_string_new("");
>  -    g_string_append_printf(xx, "0x%" PRIx64 ": %s - misses: %lu\n",
>  -            insn->addr, insn->disas_str, insn->misses);
>  +    g_string_append_printf(xx, "0x%" PRIx64, insn->addr);
>  +    if (insn->symbol) {
>  +        g_string_append_printf(xx, " (%s)", insn->symbol);
>  +    }
>  +    g_string_append_printf(xx, ", %lu, %s\n", insn->misses, insn->disas_str);
>       qemu_plugin_outs(xx->str);
>   }
>
>  diff --git a/plugins/api.c b/plugins/api.c
>  index 817c9b6b69..332e2c60e2 100644
>  --- a/plugins/api.c
>  +++ b/plugins/api.c
>  @@ -233,6 +233,12 @@ char *qemu_plugin_insn_disas(const struct qemu_plugin_insn *insn)
>       return plugin_disas(cpu, insn->vaddr, insn->data->len);
>   } 
>
>  +const char *qemu_plugin_insn_symbol(const struct qemu_plugin_insn *insn)
>  +{
>  +    const char *sym = lookup_symbol(insn->vaddr);
>  +    return sym[0] != 0 ? sym : NULL;
>  +}
>  +
>   /*
>    * The memory queries allow the plugin to query information about a
>    * memory access.
>  -- 
>  2.20.1
>
> How can I address such an addition? Should I add it to my next RFC series
> under your name using your Signed-off-by line?

Yes, if you git am the patch to your series it will keep my authorship
and also copy the message-id of the post it came from. When you re-base
you can add your s-o-b tag to indicate that to the best of your
knowledge it is code that can be included in the tree (which it is, I
posted it to qemu-devel ;-).

> Also, I think that somethings
> in my series that you're basing your patch on will be changed, such as having
> two duplicated entries of InsnData and the stupid name "xx" of the report string
> How can I base your patch after my edits?

You have two choices. Move the patch to the start of your series and
drop the actual plugin tweaks and make it API only or keep it at the end
of the series and fix it up when you re-base. Generally you make a note
"under the line" of the changes so the commit message would look like:

  plugins/api: expose symbol lookup to plugins

  This is a quality of life helper for plugins so they don't need to
  re-implement symbol lookup when dumping an address. The strings are
  constant so don't need to be duplicated. One minor tweak is to return
  NULL instead of a zero length string to show lookup failed.

  Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
  Message-Id: <20210601145824.3849-1-alex.bennee@linaro.org>
  Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>

  ---
  v3
    - included in my tree, fixed up rebase conflicts

This is a useful reference for people reading the patches on the list to
see what changed. The tooling will drop everything under --- when it is
applied to a tree.

-- 
Alex Bennée