Re: [Qemu-devel] [PATCH] Add a DTrace tracing backend targetted for SystemTAP compatability

[Stefan Hajnoczi]

On Mon, Oct 18, 2010 at 3:04 PM, Daniel P. Berrange <address@hidden> 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 <address@hidden>
> ---
>  .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")
>       address@hidden -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