arkadiy.iva...@ispras.ru writes:
> Alex Bennée писал 2021-10-12 13:36:
>> Arkadiy <arkaisp2...@gmail.com> writes:
>>
>>> From: NDNF <arkaisp2...@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.iva...@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.iva...@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.
>
> I couldn't find a detailed description of the lcov format. Also, my
> tests with drcov2lcov were unsuccessful. At the moment, I don't think
> this is a priority.
That's a shame but I agree it shouldn't hold up getting this in ;-)
--
Alex Bennée
