On 2020-06-30 at 11:05 CEST, Dr. David Alan Gilbert wrote...
> * Christophe de Dinechin (dinec...@redhat.com) wrote:
>> The recorder library provides support for low-cost continuous
>> recording of events, which can then be replayed. This makes it
>> possible to collect data "after the fact",for example to show the
>> events that led to a crash.
>>
>> Recorder support in qemu is implemented using the existing tracing
>> interface. In addition, it is possible to individually enable
>> recorders that are not traces, although this is probably not
>> recommended.
>>
>> HMP COMMAND:
>> The 'recorder' hmp command has been added, which supports two
>> sub-commands:
>> - recorder dump: Dump the current state of the recorder. You can
> ^^^^^^^^
> is that intended?
No. I think the intent was to indicate that you could pass the name of a
recorder to dump as an arg. I'll fix.
>
>> - recorder trace: Set traces using the recorder_trace_set() syntax.
>> You can use "recorder trace help" to list all available recorders.
>>
>> Signed-off-by: Christophe de Dinechin <dinec...@redhat.com>
>> ---
>> configure | 5 +++
>> hmp-commands.hx | 19 ++++++++--
>> monitor/misc.c | 27 ++++++++++++++
>> scripts/tracetool/backend/recorder.py | 51 +++++++++++++++++++++++++++
>> trace/Makefile.objs | 2 ++
>> trace/control.c | 7 ++++
>> trace/recorder.c | 22 ++++++++++++
>> trace/recorder.h | 34 ++++++++++++++++++
>> util/module.c | 8 +++++
>> 9 files changed, 173 insertions(+), 2 deletions(-)
>> create mode 100644 scripts/tracetool/backend/recorder.py
>> create mode 100644 trace/recorder.c
>> create mode 100644 trace/recorder.h
>>
>> diff --git a/configure b/configure
>> index ae8737d5a2..130630b98f 100755
>> --- a/configure
>> +++ b/configure
>> @@ -7702,6 +7702,11 @@ fi
>> if have_backend "log"; then
>> echo "CONFIG_TRACE_LOG=y" >> $config_host_mak
>> fi
>> +if have_backend "recorder"; then
>> + echo "CONFIG_TRACE_RECORDER=y" >> $config_host_mak
>> + # This is a bit brutal, but there is currently a bug in the makefiles
>> + LIBS="$LIBS -lrecorder"
>> +fi
>> if have_backend "ust"; then
>> echo "CONFIG_TRACE_UST=y" >> $config_host_mak
>> fi
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 60f395c276..565f518d4b 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -297,6 +297,22 @@ ERST
>> .cmd = hmp_trace_file,
>> },
>>
>> +SRST
>> +``trace-file on|off|flush``
>> + Open, close, or flush the trace file. If no argument is given, the
>> + status of the trace file is displayed.
>> +ERST
>> +#endif
>> +
>> +#if defined(CONFIG_TRACE_RECORDER)
>> + {
>> + .name = "recorder",
>> + .args_type = "op:s?,arg:F?",
>
> Having 'arg' as a filename is a bit odd; using op/arg is very generic
> for adding extra commands; but it suddenly becomes less generic if
> arg is always a filename.
>
>> + .params = "trace|dump [arg]",
>> + .help = "trace selected recorders or print recorder dump",
>> + .cmd = hmp_recorder,
>> + },
>> +
>> SRST
>> ``trace-file on|off|flush``
>> Open, close, or flush the trace file. If no argument is given, the
>
> I think this SRST chunk is the one that needs updating for recorder.
> (The diff has made a bit of a mess, but I think you've copy pasted the
> trace-file chunk, but forgotten to update the SRST section).
Indeed. I had forgotten to get back to the .hx file and add the documentation.
>
>> @@ -1120,7 +1136,7 @@ ERST
>>
>> SRST
>> ``dump-guest-memory [-p]`` *filename* *begin* *length*
>> - \
>> + \
>> ``dump-guest-memory [-z|-l|-s|-w]`` *filename*
>> Dump guest memory to *protocol*. The file can be processed with crash or
>> gdb. Without ``-z|-l|-s|-w``, the dump format is ELF.
>> @@ -1828,4 +1844,3 @@ ERST
>> .sub_table = hmp_info_cmds,
>> .flags = "p",
>> },
>> -
>> diff --git a/monitor/misc.c b/monitor/misc.c
>> index 89bb970b00..0094b1860f 100644
>> --- a/monitor/misc.c
>> +++ b/monitor/misc.c
>> @@ -61,6 +61,9 @@
>> #ifdef CONFIG_TRACE_SIMPLE
>> #include "trace/simple.h"
>> #endif
>> +#ifdef CONFIG_TRACE_RECORDER
>> +#include "trace/recorder.h"
>> +#endif
>> #include "exec/memory.h"
>> #include "exec/exec-all.h"
>> #include "qemu/option.h"
>> @@ -227,6 +230,30 @@ static void hmp_trace_file(Monitor *mon, const QDict
>> *qdict)
>> }
>> #endif
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> +static void hmp_recorder(Monitor *mon, const QDict *qdict)
>> +{
>> + const char *op = qdict_get_try_str(qdict, "op");
>> + const char *arg = qdict_get_try_str(qdict, "arg");
>> +
>> + if (!op) {
>> + monitor_printf(mon, "missing recorder command\"%s\"\n", op);
>> + help_cmd(mon, "recorder");
>> + } else if (!strcmp(op, "trace")) {
>> + recorder_trace_set(arg);
>> + } else if (!strcmp(op, "dump")) {
>> + if (!arg || !*arg) {
>> + recorder_dump();
>> + } else {
>> + recorder_dump_for(arg);
>> + }
>> + } else {
>> + monitor_printf(mon, "unexpected recorder command \"%s\"\n", op);
>> + help_cmd(mon, "recorder");
>> + }
>> +}
>
> Consider whether just doing two separate commands would be easier;
> a recorder-trace and recorder-dump for example; that's fine from a HMP
> point of view and I think you'll find it's less code.
Hmmm. Looking back at the code, I see that I should also probably name the
commands something beginning with trace. Maybe trace-record and trace-dump?
>
> Dave
>
>> +#endif
>> +
>> static void hmp_info_help(Monitor *mon, const QDict *qdict)
>> {
>> help_cmd(mon, "info");
>> diff --git a/scripts/tracetool/backend/recorder.py
>> b/scripts/tracetool/backend/recorder.py
>> new file mode 100644
>> index 0000000000..79cc6f5b03
>> --- /dev/null
>> +++ b/scripts/tracetool/backend/recorder.py
>> @@ -0,0 +1,51 @@
>> +# -*- coding: utf-8 -*-
>> +
>> +"""
>> +Trace back-end for recorder library
>> +"""
>> +
>> +__author__ = "Christophe de Dinechin <christo...@dinechin.org>"
>> +__copyright__ = "Copyright 2020, Christophe de Dinechin and Red Hat"
>> +__license__ = "GPL version 2 or (at your option) any later version"
>> +
>> +__maintainer__ = "Christophe de Dinechin"
>> +__email__ = "christo...@dinechin.org"
>> +
>> +
>> +from tracetool import out
>> +
>> +PUBLIC = True
>> +
>> +def generate_h_begin(events, group):
>> + out('#include <recorder/recorder.h>', '')
>> +
>> + for event in events:
>> + out('RECORDER_DECLARE(%(name)s);', name=event.name)
>> +
>> +
>> +def generate_h(event, group):
>> + argnames = ", ".join(event.args.names())
>> + if len(event.args) > 0:
>> + argnames = ", " + argnames
>> +
>> + out(' record(%(event)s, %(fmt)s %(argnames)s);',
>> + event=event.name,
>> + fmt=event.fmt.rstrip("\n"),
>> + argnames=argnames)
>> +
>> +
>> +def generate_h_backend_dstate(event, group):
>> + out(' RECORDER_TWEAK(%(event_id)s) || \\', event_id=event.name)
>> +
>> +def generate_c_begin(events, group):
>> + out('#include "qemu/osdep.h"',
>> + '#include "trace/control.h"',
>> + '#include "trace/simple.h"',
>> + '#include <recorder/recorder.h>',
>> + '')
>> +
>> + for event in events:
>> + out('RECORDER_DEFINE(%(name)s, 8, "Tracetool recorder for
>> %(api)s(%(args)s)");',
>> + name=event.name,
>> + api=event.api(),
>> + args=event.args)
>> diff --git a/trace/Makefile.objs b/trace/Makefile.objs
>> index c544509adf..9e347640c2 100644
>> --- a/trace/Makefile.objs
>> +++ b/trace/Makefile.objs
>> @@ -54,6 +54,8 @@ $(obj)/generated-tcg-tracers.h-timestamp:
>> $(SRC_PATH)/trace-events $(BUILD_DIR)/
>>
>> util-obj-$(CONFIG_TRACE_SIMPLE) += simple.o
>> util-obj-$(CONFIG_TRACE_FTRACE) += ftrace.o
>> +util-obj-$(CONFIG_TRACE_RECORDER) += recorder.o
>> +recorder.o-libs = -lrecorder
>> util-obj-y += control.o
>> obj-y += control-target.o
>> util-obj-y += qmp.o
>> diff --git a/trace/control.c b/trace/control.c
>> index 2ffe000818..15e5293eec 100644
>> --- a/trace/control.c
>> +++ b/trace/control.c
>> @@ -23,6 +23,9 @@
>> #ifdef CONFIG_TRACE_SYSLOG
>> #include <syslog.h>
>> #endif
>> +#ifdef CONFIG_TRACE_RECORDER
>> +#include "trace/recorder.h"
>> +#endif
>> #include "qapi/error.h"
>> #include "qemu/error-report.h"
>> #include "qemu/config-file.h"
>> @@ -282,6 +285,10 @@ bool trace_init_backends(void)
>> openlog(NULL, LOG_PID, LOG_DAEMON);
>> #endif
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> + recorder_trace_init();
>> +#endif
>> +
>> return true;
>> }
>>
>> diff --git a/trace/recorder.c b/trace/recorder.c
>> new file mode 100644
>> index 0000000000..cbc22ee2d5
>> --- /dev/null
>> +++ b/trace/recorder.c
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Recorder-based trace backend
>> + *
>> + * Copyright Red Hat 2020
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "trace/recorder.h"
>> +
>> +RECORDER_CONSTRUCTOR
>> +void recorder_trace_init(void)
>> +{
>> + recorder_trace_set(getenv("RECORDER_TRACES"));
>> +
>> + // Allow a dump in case we receive some unhandled signal
>> + // For example, send USR2 to a hung process to get a dump
>> + if (getenv("RECORDER_TRACES"))
>> + recorder_dump_on_common_signals(0,0);
>> +}
>> diff --git a/trace/recorder.h b/trace/recorder.h
>> new file mode 100644
>> index 0000000000..00b11a2d2f
>> --- /dev/null
>> +++ b/trace/recorder.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Recorder-based trace backend
>> + *
>> + * Copyright Red Hat 2020
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef TRACE_RECORDER_H
>> +#define TRACE_RECORDER_H
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#ifdef CONFIG_TRACE_RECORDER
>> +
>> +#include <recorder/recorder.h>
>> +
>> +extern void recorder_trace_init(void);
>> +
>> +#else
>> +
>> +// Disable recorder macros
>> +#define RECORDER(Name, Size, Description)
>> +#define RECORDER_DEFINE(Name, Size, Description)
>> +#define RECORDER_DECLARE(Name)
>> +#define RECORD(Name, ...)
>> +#define record(Name, ...)
>> +#define recorder_trace_init()
>> +
>> +#endif // CONFIG_TRACE_RECORDER
>> +
>> +#endif // TRACE_RECORDER_H
>> diff --git a/util/module.c b/util/module.c
>> index e48d9aacc0..2fa93561fe 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -22,6 +22,10 @@
>> #ifdef CONFIG_MODULE_UPGRADES
>> #include "qemu-version.h"
>> #endif
>> +#ifdef CONFIG_TRACE_RECORDER
>> +#include "trace/recorder.h"
>> +#endif
>> +
>>
>> typedef struct ModuleEntry
>> {
>> @@ -150,6 +154,10 @@ static int module_load_file(const char *fname)
>> g_module_close(g_module);
>> ret = -EINVAL;
>> } else {
>> +#ifdef CONFIG_TRACE_RECORDER
>> + // New recorders may have been pulled in, activate them if necessary
>> + recorder_trace_init();
>> +#endif
>> QTAILQ_FOREACH(e, &dso_init_list, node) {
>> e->init();
>> register_module_init(e->init, e->type);
>> --
>> 2.26.2
>>
--
Cheers,
Christophe de Dinechin (IRC c3d)
