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

[Philippe Mathieu-Daudé]

On 6/2/21 10:43 AM, Alex Bennée wrote:
> 
> 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>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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.
>