From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1E5C4C433F5 for ; Tue, 12 Oct 2021 11:25:01 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B458D60F11 for ; Tue, 12 Oct 2021 11:25:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B458D60F11 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:55620 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1maFtz-0005bc-SN for qemu-devel@archiver.kernel.org; Tue, 12 Oct 2021 07:24:59 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58540) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1maFXm-0001Vz-RO for qemu-devel@nongnu.org; Tue, 12 Oct 2021 07:02:09 -0400 Received: from mail-wr1-x433.google.com ([2a00:1450:4864:20::433]:34371) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1maFXc-0003K0-7W for qemu-devel@nongnu.org; Tue, 12 Oct 2021 07:02:01 -0400 Received: by mail-wr1-x433.google.com with SMTP id y3so32649562wrl.1 for ; Tue, 12 Oct 2021 04:01:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=references:user-agent:from:to:cc:subject:date:in-reply-to :message-id:mime-version:content-transfer-encoding; bh=ZVQduUnw0BkoTX6+6+YBaMqhCqs93R+X6RMvgB/acMw=; b=xUhoF5xZzAApJjHo3U0JgQQALNCEhGcj8rQT0u982KyH8LTEeDkQzLMpjvvUZFroRG hqKy4F0ogUNwba7DNuwKiugchOeqyJPd9eaYUp1dlWnCu9QAohp6MmOqHjHvThVLxjvi 6TXVaME6rsvCn5jirCqZqDS8o/JYxp529Z5N480e+fDe+RPOAntp2WlEeFlhJZGHK+9k z+5EkPQKWlYgb7ryE/izDlwcBthWARTby/9xuwUC0MqAxFRZOgI+qVKP4NoDRhhWAOHc +ItUsDnz0Iac2IMhsY5Upx5Kg+S6HY341dFP1PZJNnrCwTvZbIY8RWKUEJwNia+hnFRm LVpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:references:user-agent:from:to:cc:subject:date :in-reply-to:message-id:mime-version:content-transfer-encoding; bh=ZVQduUnw0BkoTX6+6+YBaMqhCqs93R+X6RMvgB/acMw=; b=3bqeXeUIeJQLk+rEHu/x4NdV3xVLKacxeRmK+H68USKcymUAYZrStL/lUhX6g4jO8d PCwXxcBZy8fV4HQNxutdroF72FJ9iEO105jovO1kSyYIk7JwGr/+Lu0lgQrxHm+uSXsF MVT0PWdBfA9rJOZ1FK9wOURME1AOac/KM3nWpKWHiP4KCNVygbS3SD9A85GT3a9+KAN/ 1P0Aud6gEUOiltLqn3pzxiXWJJYgzJJ5ZqMva4nvSPXS/2nUcEU0nWQwz4W0x9zh32sD qpSzi2XpzRfUM34xN7weXoZLtbnVEozSSOZZhncW31vq9QeTUCNWdLB0z/hnSz6P17bf bR2A== X-Gm-Message-State: AOAM530uop5oRqtxEgAZKF7OBYe7l8OQhE5p9qdCtQpJG1GVJiT0Dcna 3wHnL6iZXqQfA++TxptE8wv/2A== X-Google-Smtp-Source: ABdhPJzraE23dTclptsSLiRH0Az/HCojnD8fri5EizrEAWgP0kAv8AD9oS7GDHwzQRLkyRpxi58j1g== X-Received: by 2002:adf:a45c:: with SMTP id e28mr31173349wra.347.1634036508789; Tue, 12 Oct 2021 04:01:48 -0700 (PDT) Received: from zen.linaroharston ([51.148.130.216]) by smtp.gmail.com with ESMTPSA id e9sm2192916wme.37.2021.10.12.04.01.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Oct 2021 04:01:47 -0700 (PDT) Received: from zen (localhost [127.0.0.1]) by zen.linaroharston (Postfix) with ESMTP id 078831FF96; Tue, 12 Oct 2021 12:01:47 +0100 (BST) References: <20211011111130.170178-1-arkaisp2021@gmail.com> User-agent: mu4e 1.7.0; emacs 28.0.60 From: Alex =?utf-8?Q?Benn=C3=A9e?= To: Arkadiy Subject: Re: [PATCH] contrib/plugins: add a drcov plugin Date: Tue, 12 Oct 2021 11:36:30 +0100 In-reply-to: <20211011111130.170178-1-arkaisp2021@gmail.com> Message-ID: <87pmsawlph.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=2a00:1450:4864:20::433; envelope-from=alex.bennee@linaro.org; helo=mail-wr1-x433.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Ivanov Arkady , qemu-devel@nongnu.org, pavel.dovgaluk@ispras.ru Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Arkadiy writes: > From: NDNF > > 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 > --- > 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 +=3D hotblocks > NAMES +=3D hotpages > NAMES +=3D howvec > NAMES +=3D lockstep > +NAMES +=3D drcov >=20=20 > SONAMES :=3D $(addsuffix .so,$(addprefix lib,$(NAMES))) >=20=20 > 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +QEMU_PLUGIN_EXPORT int qemu_plugin_version =3D QEMU_PLUGIN_VERSION; > + > +static char header[] =3D "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 =3D g_string_new(""); > + g_string_append(head, header); You could just initialise with your header: g_autoptr(GString) head =3D 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 st= ring? > + g_string_append_printf(head, "BB Table: %d bbs\n", g_slist_length(bb= s)); > + fwrite(head->str, sizeof(char), head->len, fp); > +} > + > +static void printfCharArray32(uint32_t data) > +{ > + const uint8_t *bytes =3D (const uint8_t *)(&data); > + fwrite(bytes, sizeof(char), sizeof(data), fp); > +} > + > +static void printfCharArray16(uint16_t data) > +{ > + const uint8_t *bytes =3D (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 =3D (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 =3D 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 =3D g_new0(bb_entry_t, 1); > + uint64_t pc =3D qemu_plugin_tb_vaddr(tb); > + > + size_t n =3D qemu_plugin_tb_n_insns(tb); > + for (int i =3D 0; i < n; i++) { > + bb->size +=3D qemu_plugin_insn_size(qemu_plugin_tb_get_insn(tb, = i)); > + } > + > + bb->start =3D pc; > + bb->mod_id =3D 0; > + bbs =3D 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. --=20 Alex Benn=C3=A9e