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

[Mahmoud Mandour <ma.mandourr@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4158 bytes --]

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

Thanks,
Mahmoud

[-- Attachment #2: Type: text/html, Size: 5568 bytes --]