Re: [PATCH v2 2/2] contrib/plugins: add a drcov plugin

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

NDNF <arkaisp2021@gmail.com> writes:

> This patch adds the ability to generate files in drcov format.
> Primary goal this script is to have coverage
> logfiles thatwork in Lighthouse.
>
> Signed-off-by: Ivanov Arkady <arkadiy.ivanov@ispras.ru>
> ---
>  contrib/plugins/Makefile |    1 
>  contrib/plugins/drcov.c  |  148 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 149 insertions(+)
>  create mode 100644 contrib/plugins/drcov.c
>
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> index 7801b08b0d..0a681efeec 100644
> --- a/contrib/plugins/Makefile
> +++ b/contrib/plugins/Makefile
> @@ -17,6 +17,7 @@ NAMES += hotblocks
>  NAMES += hotpages
>  NAMES += howvec
>  NAMES += lockstep
> +NAMES += drcov
>  
>  SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
>  
> diff --git a/contrib/plugins/drcov.c b/contrib/plugins/drcov.c
> new file mode 100644
> index 0000000000..f94b52ff64
> --- /dev/null
> +++ b/contrib/plugins/drcov.c
> @@ -0,0 +1,148 @@
> +/*
> + * Copyright (C) 2021, Ivanov Arkady <arkadiy.ivanov@ispras.ru>
> + *
> + * Drcov - a DynamoRIO-based tool that collects coverage information
> + * from a binary. Primary goal this script is to have coverage log
> + * files that work in Lighthouse.
> + *
> + * License: GNU GPL, version 2 or later.
> + *   See the COPYING file in the top-level directory.
> + */
> +
> +#include <inttypes.h>
> +#include <assert.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <glib.h>
> +
> +#include <qemu-plugin.h>
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
> +static char header[] = "DRCOV VERSION: 2\n"
> +                "DRCOV FLAVOR: drcov-64\n"
> +                "Module Table: version 2, count 1\n"
> +                "Columns: id, base, end, entry, path\n";
> +
> +static FILE *fp;
> +static char *file_name;
> +static GMutex lock;
> +
> +typedef struct {
> +    uint32_t start;
> +    uint16_t size;
> +    uint16_t mod_id;
> +} bb_entry_t;
> +
> +static GSList *bbs;
> +
> +static void printf_header(void)
> +{
> +    g_autoptr(GString) head = g_string_new(header);
> +    const char *path = qemu_plugin_path_to_binary();
> +    uint64_t start_code = qemu_plugin_start_code();
> +    uint64_t end_code = qemu_plugin_end_code();
> +    uint64_t entry = qemu_plugin_entry_code();
> +    g_string_append_printf(head, "0, 0x%lx, 0x%lx, 0x%lx, %s\n",
> +                           start_code, end_code, entry, path);
> +    g_string_append_printf(head, "BB Table: %d bbs\n", g_slist_length(bbs));
> +    fwrite(head->str, sizeof(char), head->len, fp);
> +}
> +
> +static void printf_char_array32(uint32_t data)
> +{
> +    const uint8_t *bytes = (const uint8_t *)(&data);
> +    fwrite(bytes, sizeof(char), sizeof(data), fp);
> +}
> +
> +static void printf_char_array16(uint16_t data)
> +{
> +    const uint8_t *bytes = (const uint8_t *)(&data);
> +    fwrite(bytes, sizeof(char), sizeof(data), fp);
> +}
> +
> +
> +static void printf_el(gpointer data, gpointer user_data)
> +{
> +    g_mutex_lock(&lock);
> +
> +    bb_entry_t *bb = (bb_entry_t *)data;
> +    printf_char_array32(bb->start);
> +    printf_char_array16(bb->size);
> +    printf_char_array16(bb->mod_id);
> +
> +    g_mutex_unlock(&lock);
> +}
> +
> +static void plugin_exit(qemu_plugin_id_t id, void *p)
> +{
> +    /* Print function */
> +    printf_header();
> +    g_slist_foreach(bbs, printf_el, NULL);
> +
> +    /* Clear */
> +    g_slist_free_full(bbs, &g_free);
> +    g_free(file_name);
> +    fclose(fp);
> +}
> +
> +static void plugin_init(void)
> +{
> +    fp = fopen(file_name, "wb");
> +}
> +
> +static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
> +{
> +    bb_entry_t *bb = g_malloc0(sizeof(bb_entry_t));
> +    memcpy(bb, udata, sizeof(bb_entry_t));
> +
> +    g_mutex_lock(&lock);
> +    bbs = g_slist_append(bbs, bb);
> +    g_mutex_unlock(&lock);

Hmm given the data you are copying is static and never going to change
you could just save the pointer to it in a log. Instead of maintaining a
bbs g_slist you could use g_ptr_array_add(bbs, udata) (with appropriate
locking) and then just iterate over the log at the end. As the same
entry will have multiple references you probably need another ptr array
just to track the creation bellow which you can use to clean-up on exit.
That should still be a net saving on two mallocs and s_list.

> +}
> +
> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> +{
> +    bb_entry_t *bb = g_new0(bb_entry_t, 1);
> +    uint64_t pc = qemu_plugin_tb_vaddr(tb);
> +
> +    size_t n = qemu_plugin_tb_n_insns(tb);
> +    for (int i = 0; i < n; i++) {
> +        bb->size += qemu_plugin_insn_size(qemu_plugin_tb_get_insn(tb, i));
> +    }
> +
> +    bb->start = pc;
> +    bb->mod_id = 0;
> +
> +    qemu_plugin_register_vcpu_tb_exec_cb(tb, vcpu_tb_exec,
> +                                         QEMU_PLUGIN_CB_NO_REGS,
> +                                         (void *)bb);
> +
> +}
> +
> +QEMU_PLUGIN_EXPORT
> +int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
> +                        int argc, char **argv)
> +{
> +
> +    if (!argc) {
> +        file_name = "file.drcov.trace";
> +    } else {
> +        if (g_str_has_prefix(argv[0], "filename=")) {
> +            size_t size = strlen(argv[0]) - 9;
> +            file_name = g_malloc0(size + 1);
> +            strncpy(file_name, argv[0] + 9, size);
> +            file_name[size] = '\0';

I guess it's probably overkill for a single arg but if you were looping
of argv something like:

        g_autofree char **tokens = g_strsplit(argv[i], "=", 2);
        if (g_strcmp0(tokens[0], "filename") == 0) {
            file_name = g_strdup(tokens[1]);
        }

would be a more glib-like way to deal with it.

> +        }
> +    }
> +
> +    plugin_init();
> +
> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +
> +    return 0;
> +}


-- 
Alex Bennée