On Mon, Oct 18, 2010 at 3:04 PM, Daniel P. Berrange <berra...@redhat.com> wrote:
> This introduces a new tracing backend that targets the SystemTAP
> implementation of DTrace userspace tracing. The core functionality
> should be applicable and standard across any DTrace implementation
> on Solaris, OS-X, *BSD, but the Makefile rules will likely need
> some small additional changes to cope with OS specific build
> requirements.
Cool, I will try this patch out shortly. Here a few comments:
DTrace detection in ./configure would help users trying out
--trace-backend=dtrace on systems without SystemTAP installed.
Perhaps running dtrace(1) is a sufficient test? If SystemTAP is not
installed then an error message from ./configure will save users time.
>
> This backend builds a little differently from the other tracing
> backends. Specifically there is no 'trace.c' file, because the
> 'dtrace' command line tool generates a '.o' file directly from
> the dtrace probe definition file. The probe definition is usually
> named with a '.d' extension but QEMU uses '.d' files for its
> external makefile dependancy tracking, so this uses '.dtrace' as
> the extension for the probe definition file.
>
> The 'tracetool' program gains the ability to generate a trace.h
> file for DTrace, and also to generate the trace.d file containing
> the dtrace probe definition, and finally a qemu.stp file which is
> a wrapper around the probe definition providing more convenient
> access from SystemTAP scripts.
>
> eg, instead of
>
> probe process("qemu").mark("qemu_malloc") {
> printf("Malloc %d %p\n", $arg1, $arg2);
> }
>
> The addition of qemu.stp to /usr/share/systemtap/tapset/
> lets users write
>
> probe qemu.qemu_malloc {
> printf("Malloc %d %p\n", size, ptr);
> }
>
> * .gitignore: Ignore trace-dtrace.*
> * Makefile: Extra rules for generating DTrace files
> * Makefile.obj: Don't build trace.o for DTrace, use
> trace-dtrace.o generated by 'dtrace' instead
> * tracetool: Support for generating DTrace/SystemTAP
> data files
>
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> ---
> .gitignore | 3 +
> Makefile | 31 ++++++++++
> Makefile.objs | 4 +
> tracetool | 175
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 211 insertions(+), 2 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index a43e4d1..0d27afd 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -4,6 +4,9 @@ config-host.*
> config-target.*
> trace.h
> trace.c
> +trace-dtrace.h
> +trace-dtrace.dtrace
> +qemu.stp
> *-timestamp
> *-softmmu
> *-darwin-user
> diff --git a/Makefile b/Makefile
> index 252c817..812b0d3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,6 +1,9 @@
> # Makefile for QEMU.
>
> GENERATED_HEADERS = config-host.h trace.h
> +ifeq ($(TRACE_BACKEND),dtrace)
> +GENERATED_HEADERS += trace-dtrace.h
> +endif
>
> ifneq ($(wildcard config-host.mak),)
> # Put the all: rule here so that config-host.mak can contain dependencies.
> @@ -106,7 +109,11 @@ ui/vnc.o: QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
>
> bt-host.o: QEMU_CFLAGS += $(BLUEZ_CFLAGS)
>
> +ifeq ($(TRACE_BACKEND),dtrace)
> +trace.h: trace.h-timestamp trace-dtrace.h
> +else
> trace.h: trace.h-timestamp
> +endif
> trace.h-timestamp: $(SRC_PATH)/trace-events config-host.mak
> $(call quiet-command,sh $(SRC_PATH)/tracetool --$(TRACE_BACKEND) -h <
> $< > $@," GEN trace.h")
> �...@cmp -s $@ trace.h || cp $@ trace.h
> @@ -118,6 +125,23 @@ trace.c-timestamp: $(SRC_PATH)/trace-events
> config-host.mak
>
> trace.o: trace.c $(GENERATED_HEADERS)
>
> +trace-dtrace.h: trace-dtrace.dtrace
> + $(call quiet-command,dtrace -o $@ -h -s $<, " GEN trace-dtrace.h")
> +
> +# Normal practice is to name DTrace probe file with a '.d' extension
> +# but that gets picked up by QEMU's Makefile as an external dependancy
> +# rule file. So we use '.dtrace' instead
> +trace-dtrace.dtrace: trace-dtrace.dtrace-timestamp
> +trace-dtrace.dtrace-timestamp: $(SRC_PATH)/trace-events config-host.mak
> + $(call quiet-command,sh $(SRC_PATH)/tracetool --$(TRACE_BACKEND) -d <
> $< > $@," GEN trace-dtrace.dtrace")
> + @cmp -s $@ trace-dtrace.dtrace || cp $@ trace-dtrace.dtrace
> +ifdef CONFIG_LINUX
> + $(call quiet-command,sh $(SRC_PATH)/tracetool --$(TRACE_BACKEND) -s <
> $< > qemu.stp," GEN qemu.stp")
> +endif
> +
> +trace-dtrace.o: trace-dtrace.dtrace $(GENERATED_HEADERS)
> + $(call quiet-command,dtrace -o $@ -G -s $<, " GEN trace-dtrace.o")
> +
> simpletrace.o: simpletrace.c $(GENERATED_HEADERS)
>
> version.o: $(SRC_PATH)/version.rc config-host.mak
> @@ -154,6 +178,7 @@ clean:
> rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d
> net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d
> rm -f qemu-img-cmds.h
> rm -f trace.c trace.h trace.c-timestamp trace.h-timestamp
> + rm -f trace-dtrace.dtrace trace-dtrace.h trace-dtrace.h-timestamp
> qemu.stp
> $(MAKE) -C tests clean
> for d in $(ALL_SUBDIRS) libhw32 libhw64 libuser libdis libdis-user; do
> \
> if test -d $$d; then $(MAKE) -C $$d $@ || exit 1; fi; \
> @@ -214,6 +239,12 @@ ifneq ($(BLOBS),)
> $(INSTALL_DATA) $(SRC_PATH)/pc-bios/$$x
> "$(DESTDIR)$(datadir)"; \
> done
> endif
> +ifeq ($(TRACE_BACKEND),dtrace)
> +ifdef CONFIG_LINUX
> + $(INSTALL_DIR) "$(DESTDIR)$(datadir)/../systemtap/tapset"
> + $(INSTALL_DATA) qemu.stp "$(DESTDIR)$(datadir)/../systemtap/tapset"
> +endif
> +endif
> $(INSTALL_DIR) "$(DESTDIR)$(datadir)/keymaps"
> set -e; for x in $(KEYMAPS); do \
> $(INSTALL_DATA) $(SRC_PATH)/pc-bios/keymaps/$$x
> "$(DESTDIR)$(datadir)/keymaps"; \
> diff --git a/Makefile.objs b/Makefile.objs
> index 816194a..ccecda0 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -273,10 +273,14 @@ libdis-$(CONFIG_SPARC_DIS) += sparc-dis.o
> ######################################################################
> # trace
>
> +ifeq ($(TRACE_BACKEND),dtrace)
> +trace-obj-y = trace-dtrace.o
> +else
> trace-obj-y = trace.o
> ifeq ($(TRACE_BACKEND),simple)
> trace-obj-y += simpletrace.o
> endif
> +endif
>
> vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>
> diff --git a/tracetool b/tracetool
> index 7010858..59b66fd 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -20,10 +20,13 @@ Backends:
> --nop Tracing disabled
> --simple Simple built-in backend
> --ust LTTng User Space Tracing backend
> + --dtrace DTrace/SystemTAP backend
>
> Output formats:
> -h Generate .h file
> -c Generate .c file
> + -d Generate .d file (DTrace only)
> + -s Generate .stp file (DTrace with SystemTAP only)
> EOF
> exit 1
> }
> @@ -68,6 +71,31 @@ get_argnames()
> fi
> }
>
> +# Get the argument name list of a trace event
> +get_arglist()
The only difference between get_arglist() and get_argnames() is the
comma added on the end of each argument name. Please reuse
get_argnames() instead of duplicating it. One solution is to pass the
argument separator as a string into get_argnames(). Passing '"' would
produce the existing behavior and passing "" would produce the
get_arglist() behavior that you need.
> +{
> + local nfields field name
> + nfields=0
> + for field in $(get_args "$1"); do
> + nfields=$((nfields + 1))
> +
> + # Drop pointer star
> + field=${field#\*}
> +
> + # Only argument names have commas at the end
> + name=${field%,}
> + test "$field" = "$name" && continue
> +
> + printf "%s" "$name "
> + done
> +
> + # Last argument name
> + if [ "$nfields" -gt 1 ]
> + then
> + printf "%s" "$name"
> + fi
> +}
> +
> # Get the number of arguments to a trace event
> get_argc()
> {
> @@ -306,6 +334,127 @@ EOF
> echo "}"
> }
>
> +linetoh_begin_dtrace()
> +{
> + cat <<EOF
> +#include "trace-dtrace.h"
> +EOF
> +}
> +
> +linetoh_dtrace()
> +{
> + local name args state
Missing argnames and nameupper.
> + name=$(get_name "$1")
> + args=$(get_args "$1")
> + argnames=$(get_argnames "$1")
> + state=$(get_state "$1")
> + if [ "$state" = "0" ] ; then
> + name=${name##disable }
> + fi
> +
> + nameupper=`echo $name | tr '[:lower:]' '[:upper:]'`
> +
> + # Define an empty function for the trace event
> + cat <<EOF
> +static inline void trace_$name($args) {
> + if (QEMU_${nameupper}_ENABLED())
> + QEMU_${nameupper}($argnames);
> +}
Please follow QEMU coding style even for generated code. 4 space
indentation and curly braces for all if statements:
static inline void trace_$name($args) {
if (QEMU_${nameupper}_ENABLED()) {
QEMU_${nameupper}($argnames);
}
}
> +EOF
> +}
> +
> +linetoh_end_dtrace()
> +{
> + return
> +}
> +
> +linetoc_begin_dtrace()
> +{
> + return
> +}
> +
> +linetoc_dtrace()
> +{
> + # No need for function definitions in dtrace backend
> + return
> +}
> +
> +linetoc_end_dtrace()
> +{
> + return
> +}
> +
> +linetod_begin_dtrace()
> +{
> + cat <<EOF
> +provider qemu {
> +EOF
> +}
> +
> +linetod_dtrace()
> +{
> + local name args state
> + name=$(get_name "$1")
> + args=$(get_args "$1")
> + state=$(get_state "$1")
> + if [ "$state" = "0" ] ; then
> + name=${name##disable }
> + fi
> +
> + # Define prototype for probe arguments
> + cat <<EOF
> + probe $name($args);
> +EOF
> +}
> +
> +linetod_end_dtrace()
> +{
> + cat <<EOF
> +};
> +EOF
> +}
> +
> +linetos_begin_dtrace()
> +{
> + return
> +}
> +
> +linetos_dtrace()
> +{
> + local name args argnames state
Missing arglist. argnames is unused.
> + name=$(get_name "$1")
> + args=$(get_args "$1")
> + arglist=$(get_arglist "$1")
> + state=$(get_state "$1")
> + if [ "$state" = "0" ] ; then
> + name=${name##disable }
> + fi
> +
> + # Define prototype for probe arguments
> + cat <<EOF
> +probe qemu.$name = process("qemu").mark("$name")
> +{
> +EOF
> +
> + i=1
> + for arg in $arglist
> + do
> + cat <<EOF
> + $arg = \$arg$i;
> +EOF
> + i="$((i+1))"
> + done
> +
> + cat <<EOF
> +}
> +EOF
> +}
> +
> +linetos_end_dtrace()
> +{
> + return
> +}
> +
> # Process stdin by calling begin, line, and end functions for the backend
> convert()
> {
> @@ -326,7 +475,7 @@ convert()
> if test -z "$disable"; then
> # Pass the disabled state as an arg to lineto$1_simple().
This comment is not outdated. Please update it or change it so the
backend is not explicitly named.
Stefan
