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

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

Arkadiy <arkaisp2021@gmail.com> writes:

> From: NDNF <arkaisp2021@gmail.com>
>
> This patch adds the ability to generate files in drcov format.
> Primary goal this script is to have coverage
> logfiles thatwork in Lighthouse.
> Problems:
>     - The path to the executable file is not specified.

I don't see a problem in introducing a plugin helper function to expose
the path to the binary/kernel to the plugin.

>     - base, end, entry take incorrect values.
>       (Lighthouse + IDA Pro anyway work).

What are they meant to be? Again we could add a helper.

>
> Signed-off-by: Ivanov Arkady <arkadiy.ivanov@ispras.ru>
> ---
>  contrib/plugins/Makefile |   1 +
>  contrib/plugins/drcov.c  | 112 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 113 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..d6a7d131c0
> --- /dev/null
> +++ b/contrib/plugins/drcov.c
> @@ -0,0 +1,112 @@
> +/*
> + * 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;
> +
> +typedef struct {
> +    uint32_t start;
> +    uint16_t size;
> +    uint16_t mod_id;
> +} bb_entry_t;
> +
> +static GSList *bbs;
> +
> +static void printfHeader()

missing void in args.

> +{
> +    g_autoptr(GString) head = g_string_new("");
> +    g_string_append(head, header);

You could just initialise with your header:

  g_autoptr(GString) head = g_string_new(header);

> +    g_string_append_printf(head, "0, 0x%x, 0x%x, 0x%x, %s\n",
> +                           0, 0xffffffff, 0, "path");

Why pass consts intro the printf instead of just appending the data as a string?

> +    g_string_append_printf(head, "BB Table: %d bbs\n", g_slist_length(bbs));
> +    fwrite(head->str, sizeof(char), head->len, fp);
> +}
> +
> +static void printfCharArray32(uint32_t data)
> +{
> +    const uint8_t *bytes = (const uint8_t *)(&data);
> +    fwrite(bytes, sizeof(char), sizeof(data), fp);
> +}
> +
> +static void printfCharArray16(uint16_t data)
> +{
> +    const uint8_t *bytes = (const uint8_t *)(&data);
> +    fwrite(bytes, sizeof(char), sizeof(data), fp);
> +}

Can the above function names follow the QEMU house style please?

> +
> +
> +static void printf_el(gpointer data, gpointer user_data)
> +{
> +    bb_entry_t *bb = (bb_entry_t *)data;
> +    printfCharArray32(bb->start);
> +    printfCharArray16(bb->size);
> +    printfCharArray16(bb->mod_id);
> +}
> +
> +static void plugin_exit(qemu_plugin_id_t id, void *p)
> +{
> +    /* Print function */
> +    printfHeader();
> +    g_slist_foreach(bbs, printf_el, NULL);
> +
> +    /* Clear */
> +    g_slist_free_full(bbs, &g_free);
> +    fclose(fp);
> +}
> +
> +static void plugin_init(void)
> +{
> +    fp = fopen("file.drcov.trace", "wb");

Could we make this configurable and just have "file.drcov.trace" as the
default if not set?

> +}
> +
> +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;
> +    bbs = g_slist_append(bbs, bb);
> +
> +}

I'm guessing this works in the simple case but beware that not all
translations get executed. It might be better to as a install an actual
tracer when the TB gets executed.

Although most TBs run to completion there are cases where execution
stops in them middle of TB. Generally this will be when a synchronous
fault has occurred and we exit the block early, potentially regenerating
a block at the PC the fault was at.

The g_list_append should be protected by a mutex as translation can be
multi-threaded (at least for system emulation).

> +
> +QEMU_PLUGIN_EXPORT
> +int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info,
> +                        int argc, char **argv)
> +{
> +    plugin_init();
> +
> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +    return 0;
> +}

I'll happily take a v2 of this patch with the fixes outlined above.

However I note the dynamorio pages talk about drcov2lcov which can
convert to lcov data. Given the basics of code coverage will be the same
it would be nice to see a re-factoring that would allow for multiple
output formats so the user could select their preferred output as an
option.

-- 
Alex Bennée