[DTrace-devel] [PATCH v4 1/7] cg: move get_member() to dt_cg.c

Kris Van Hees kris.van.hees at oracle.com
Wed Jul 9 16:47:58 UTC 2025


Woops, one more.

On Wed, Jul 09, 2025 at 12:28:15PM -0400, Kris Van Hees wrote:
> Some comments below...  Sorry - I got distracted by the other patches and
> failed to give this one attention.
> 
> On Wed, Jul 09, 2025 at 03:46:54PM +0100, Alan Maguire via DTrace-devel wrote:
> > It will be used by both dt_prov_ip.c and dt_prov_tcp.c.
> > 
> > Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
> > Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> >  libdtrace/dt_cg.c      | 39 ++++++++++++++++++++++++++++++++++++
> >  libdtrace/dt_cg.h      |  2 ++
> >  libdtrace/dt_prov_ip.c | 45 ++++--------------------------------------
> >  3 files changed, 45 insertions(+), 41 deletions(-)
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index bd0763d6..d6fc8259 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -1901,6 +1901,45 @@ dt_cg_ctf_offsetof(const char *structname, const char *membername,
> >  	return (ctm.ctm_offset / NBBY);
> >  }
> >  
> > +/*
> > + * Retrieve the value of a member in a given struct.
> > + *
> > + * Entry:
> > + *	reg = TYPE *ptr
> > + *
> > + * Return:
> > + *	%r0 = ptr->member
> > + * Clobbers:
> > + *	%r1 .. %r5
> > + */
> > +int
> > +dt_cg_get_member(dt_pcb_t *pcb, const char *name, int reg,
> > +		 const char *member)

Why is dt_cg_get_member() returning int?  Nothing looks at it anyway.  ANd
with the change to use dt_cg_ctf_offset(), compilation error will be repotred
from there.

> Given that this is meant to be used from trampoline code (where we currently
> do not do register tracking - i.e. we expect the code writer to make sure that
> no collisions of register use occur), it probably should be named dt_cg_tramp_*
> rather than dt_cg_*.  That also signals people looking at this that register
> allocation is done manually, and with care, so we can expect things like %r0
> getting assigned a value here, and that we expect that the passed in reg is NOT
> %r0.
> 
> > +{
> > +	dtrace_hdl_t		*dtp = pcb->pcb_hdl;
> > +	dt_irlist_t		*dlp = &pcb->pcb_ir;
> > +	dtrace_typeinfo_t	tt;
> > +	ctf_membinfo_t		ctm;
> > +	size_t			size;
> > +	uint_t			ldop;
> > +
> > +	if (dtrace_lookup_by_type(dtp, DTRACE_OBJ_KMODS, name, &tt) == -1 ||
> > +	    ctf_member_info(tt.dtt_ctfp, tt.dtt_type, member, &ctm) == CTF_ERR)
> > +		return -1;
> 
> The dt_cg_tramp_get_member name would have me assume that this can get me the
> member of any struct, not just those defined in kernel modules (or the kernel
> proper - which in DTrace terms is a module).  So, if the intent is that it can
> only use structs defined at the kernel level, then the function name should
> probably reflect that.  If not (and that is what I would prefer), then this
> should do a lookup for DTRACE_OBJ_EVERY, like e.g. dt_cg_ctf_offsetof does.
> 
> On that note, perhaps it would be easier to extend dt_cg_ctf_offsetof() to
> take another (optional) arg to put the ldop in, populate that one if a ptr
> is passed to store it in, and then use that here?  Because you are essentially
> doing the same lookup, you need the offset, and the size, and so the only
> extra think needed (that dt_cg_ctf_offsetof() could provide easily) is the
> ldop.
> 
> So, I think dt_cg_ctf_offsetof() could become:
> 
> dt_cg_ctf_offsetof(const char *structname, const char *membername,
>                    size_t *sizep, uint_t *ldopp, int relaxed)
> 
> and instead of:
> 
>         if (sizep)
>                 *sizep = ctf_type_size(ctfp, ctm.ctm_type);
> 
> it could use:
> 
> 	if (sizep || ldopp) {
> 		uint_t	ldop;
> 
> 		ldop = dt_cg_ldsize(NULL, tt.dtt_ctfp, ctm.ctm_type, sizep);
> 		if (ldopp)
> 			*ldopp = ldop;
> 	}
> 
> and then the dt_cg_tramp_get_member can just use:
> 
> 	offset = dt_cg_ctf_offsetof(name, member, &size, &ldop, 0);
> 
> instead of the lookup above, and the dt_cg_ldsize() call below.
> 
> > +
> > +	ldop = dt_cg_ldsize(NULL, tt.dtt_ctfp, ctm.ctm_type, &size);
> > +
> > +	emit(dlp, BPF_MOV_REG(BPF_REG_3, reg));
> > +	emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, ctm.ctm_offset / NBBY));
> 
> This would use offset.
> 
> > +	emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> > +	emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> > +	emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_BASE));
> > +	emit(dlp, BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> > +	emit(dlp, BPF_LOAD(ldop, BPF_REG_0, BPF_REG_FP, DT_TRAMP_SP_BASE));
> > +
> > +	return 0;
> > +}
> > +
> >  static void
> >  dt_cg_act_breakpoint(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >  {
> > diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
> > index 81b79399..36f587d3 100644
> > --- a/libdtrace/dt_cg.h
> > +++ b/libdtrace/dt_cg.h
> > @@ -44,6 +44,8 @@ extern void dt_cg_tramp_epilogue_advance(dt_pcb_t *pcb, dt_activity_t act);
> >  extern void dt_cg_tramp_error(dt_pcb_t *pcb);
> >  extern int dt_cg_ctf_offsetof(const char *structname, const char *membername,
> >  			      size_t *sizep, int relaxed);
> > +extern int dt_cg_get_member(dt_pcb_t *pcb, const char *name, int reg,
> > +			    const char *member);
> >  extern uint_t dt_cg_ldsize(dt_node_t *dnp, ctf_file_t *ctfp, ctf_id_t type,
> >  			 ssize_t *ret_size);
> >  extern uint_t bpf_ldst_size(ssize_t size, int store);
> > diff --git a/libdtrace/dt_prov_ip.c b/libdtrace/dt_prov_ip.c
> > index c4a3a6e2..37f91e3d 100644
> > --- a/libdtrace/dt_prov_ip.c
> > +++ b/libdtrace/dt_prov_ip.c
> > @@ -62,43 +62,6 @@ static int populate(dtrace_hdl_t *dtp)
> >  			       probe_args, probes);
> >  }
> >  
> > -/*
> > - * Retrieve the value of a member in a given struct.
> > - *
> > - * Entry:
> > - *	reg = TYPE *ptr
> > - *
> > - * Return:
> > - *	%r0 = ptr->member
> > - * Clobbers:
> > - *	%r1 .. %r5
> > - */
> > -static int get_member(dt_pcb_t *pcb, const char *name, int reg,
> > -		      const char *member) {
> > -	dtrace_hdl_t		*dtp = pcb->pcb_hdl;
> > -	dt_irlist_t		*dlp = &pcb->pcb_ir;
> > -	dtrace_typeinfo_t	tt;
> > -	ctf_membinfo_t		ctm;
> > -	size_t			size;
> > -	uint_t			ldop;
> > -
> > -	if (dtrace_lookup_by_type(dtp, DTRACE_OBJ_KMODS, name, &tt) == -1 ||
> > -	    ctf_member_info(tt.dtt_ctfp, tt.dtt_type, member, &ctm) == CTF_ERR)
> > -		return -1;
> > -
> > -	ldop = dt_cg_ldsize(NULL, tt.dtt_ctfp, ctm.ctm_type, &size);
> > -
> > -	emit(dlp, BPF_MOV_REG(BPF_REG_3, reg));
> > -	emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, ctm.ctm_offset / NBBY));
> > -	emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> > -	emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_FP));
> > -	emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DT_TRAMP_SP_BASE));
> > -	emit(dlp, BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> > -	emit(dlp, BPF_LOAD(ldop, BPF_REG_0, BPF_REG_FP, DT_TRAMP_SP_BASE));
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * Generate a BPF trampoline for a SDT probe.
> >   *
> > @@ -142,7 +105,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >  
> >  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_6));
> >  
> > -	get_member(pcb, "struct sk_buff", BPF_REG_6, "sk");
> > +	dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "sk");
> >  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(1), BPF_REG_0));
> >  
> >  	/*
> > @@ -150,11 +113,11 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >  	 *	skb_network_header(skb)	=	(include/linux/ip.h)
> >  	 *	skb->head + skb->network_header	(include/linux/skbuff.h)
> >  	 */
> > -	get_member(pcb, "struct sk_buff", BPF_REG_6, "head");
> > +	dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "head");
> >  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(2), BPF_REG_0));
> >  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(4), BPF_REG_0));
> >  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(5), BPF_REG_0));
> > -	get_member(pcb, "struct sk_buff", BPF_REG_6, "network_header");
> > +	dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "network_header");
> >  	emit(dlp, BPF_XADD_REG(BPF_DW, BPF_REG_7, DMST_ARG(2), BPF_REG_0));
> >  	emit(dlp, BPF_XADD_REG(BPF_DW, BPF_REG_7, DMST_ARG(4), BPF_REG_0));
> >  	emit(dlp, BPF_XADD_REG(BPF_DW, BPF_REG_7, DMST_ARG(5), BPF_REG_0));
> > @@ -168,7 +131,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >  	else
> >  		emit(dlp, BPF_STORE_IMM(BPF_DW, BPF_REG_7, DMST_ARG(5), 0));
> >  
> > -	get_member(pcb, "struct sk_buff", BPF_REG_6, "dev");
> > +	dt_cg_get_member(pcb, "struct sk_buff", BPF_REG_6, "dev");
> >  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(3), BPF_REG_0));
> >  
> >  	return 0;
> > -- 
> > 2.39.3
> > 
> > 
> > _______________________________________________
> > DTrace-devel mailing list
> > DTrace-devel at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/dtrace-devel



More information about the DTrace-devel mailing list