[DTrace-devel] [PATCH 04/20] cg: add dt_cg_trace

[Eugene Loh]

My opinion on this... remains unchanged!  The basic functionality seems 
really attractive, but I don't like the interface.

I think it would be nice to mimic the functionality of 
bpf_trace_printk() more closely.  That is, allow three values to be 
printed.  Further, let each one be an IMM or REG.  I know we disagreed 
on what a sensible interface could look like, but that strikes me as a 
solvable problem.  The current interface with "counter" serving two very 
different purposes sets the bar low.

Further, controlling the operation with debugging= also strikes me as 
weird.  That build setting is for GCC optimization, while the current 
patch relates to BPF cg.  Actually, I could imagine just having the 
function be "on" all the time.  Let whoever is calling the function 
decide whether to call it or not.  After all, calls to the function will 
presumably be from specialized debugging sites... i imagine within some 
"#ifdef FIXME" or something.

Finally I'm curious why the BPF C file is called trace-ptr.c and why 
that argument is called ptr.  Not a big deal but "ptr" seems like a 
counterintuitive name.

Anyhow my $0.02.

On 5/11/22 14:12, Nick Alcock via DTrace-devel wrote:
> A call to
>
>     dt_cg_trace(dlp, drp, 1, foo);
>
> will trace the value of reg FOO into
> /sys/kernel/debug/tracing/trace_pipe when dtrace is compiled with
> debugging=yes.  the value '1' above is both a counter value which is
> also logged in the resulting trace output, to distinguish trace calls
> from each other, and a value which if odd means to trace a reg's
> value, and if even to trace the immediate value passed in.
>
> Under debugging=no this call has no effect and is compiled out.
>
> Signed-off-by: Nick Alcock<nick.alcock at oracle.com>
> ---
>   Makeoptions       |  3 ++-
>   bpf/Build         |  3 ++-
>   bpf/trace_ptr.c   | 23 +++++++++++++++++++++++
>   libdtrace/dt_cg.c | 33 +++++++++++++++++++++++++++++++++
>   4 files changed, 60 insertions(+), 2 deletions(-)
>   create mode 100644 bpf/trace_ptr.c
>
> diff --git a/Makeoptions b/Makeoptions
> index 011440a31d83..dba38641154f 100644
> --- a/Makeoptions
> +++ b/Makeoptions
> @@ -17,7 +17,8 @@ help::
>   	@printf "\n" >&2
>   
>   ifneq ($(debugging),no)
> -override CFLAGS += -O0 -g
> +override CFLAGS += -O0 -g -DDEBUGGING
> +override BPFCFLAGS += -DDEBUGGING
>   endif
>   
>   ifneq ($(coverage),no)
> diff --git a/bpf/Build b/bpf/Build
> index b29f4b917f59..5c021082d6fa 100644
> --- a/bpf/Build
> +++ b/bpf/Build
> @@ -45,7 +45,8 @@ bpf_dlib_SOURCES = \
>   	strlen.c \
>   	strrchr.S \
>   	strtok.S \
> -	substr.S
> +	substr.S \
> +	trace_ptr.c
>   
>   bpf-check: $(objdir)/include/.dir.stamp
>   	$(BPFC) $(BPFCPPFLAGS) $(bpf_dlib_CPPFLAGS) $(BPFCFLAGS) -S \
> diff --git a/bpf/trace_ptr.c b/bpf/trace_ptr.c
> new file mode 100644
> index 000000000000..48e07a2bd561
> --- /dev/null
> +++ b/bpf/trace_ptr.c
> @@ -0,0 +1,23 @@
> +#ifdef DEBUGGING
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + */
> +#include <linux/bpf.h>
> +#include <stdint.h>
> +#include <bpf-helpers.h>
> +
> +#ifndef noinline
> +# define noinline	__attribute__((noinline))
> +#endif
> +
> +noinline void dt_trace_ptr(uint64_t counter, uint64_t ptr)
> +{
> +	/*
> +	 * Can't use a straight string constant: DTrace cannot yet process
> +	 * rodata relocs.
> +	 */
> +	char fmt[] = "debug: %d: %lx\n";
> +	bpf_trace_printk(fmt, 16, counter, ptr);
> +}
> +#endif
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 823fe8e3082f..d35d71394a5f 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -28,6 +28,9 @@
>   #define DT_ISREG	1
>   
>   static void dt_cg_node(dt_node_t *, dt_irlist_t *, dt_regset_t *);
> +static void_dt_unused_
> +dt_cg_trace(dt_irlist_t *dlp_dt_unused_, dt_regset_t *drp_dt_unused_,
> +	    int counter_dt_unused_, uint64_t val_dt_unused_);
>   
>   /*
>    * Generate the generic prologue of the trampoline BPF program.
> @@ -764,6 +767,36 @@ dt_cg_fill_gap(dt_pcb_t *pcb, int gap)
>   		emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, off, 0));
>   }
>   
> +/*
> + * Trace an immediate counter and a value to the log at
> + * /sys/kernel/debug/tracing/trace_pipe.  If COUNTER is odd, VAL is a register
> + * number.  Otherwise, it's an immediate value.
> + */
> +static void_dt_unused_
> +dt_cg_trace(dt_irlist_t *dlp_dt_unused_, dt_regset_t *drp_dt_unused_,
> +	    int counter_dt_unused_, uint64_t val_dt_unused_)
> +{
> +#ifdef DEBUGGING
> +	dt_ident_t	*idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_trace_ptr");
> +
> +	assert(idp != NULL);
> +
> +	if (dt_regset_xalloc_args(drp) == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> +	if (counter % 2)
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_2, val));
> +	else
> +		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, val));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_1, counter));
> +
> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> +	dt_regset_free(drp, BPF_REG_0);
> +	dt_regset_free_args(drp);
> +#endif
> +}
> +
>   static void
>   dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
>   {
> -- 2.36.1.263.g194b774378.dirty 
> _______________________________________________ DTrace-devel mailing list