On 6/2/21 10:43 AM, Alex Bennée wrote:
>
> Mahmoud Mandour <ma.mando...@gmail.com> writes:
>
>> On Tue, Jun 1, 2021 at 4:58 PM Alex Bennée <alex.ben...@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.mando...@gmail.com
>> Signed-off-by: Alex Bennée <alex.ben...@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.ben...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> Message-Id: <20210601145824.3849-1-alex.ben...@linaro.org>
> Signed-off-by: Mahmoud Mandour <ma.mando...@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.
>
