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

[Kris Van Hees <kris.van.hees@oracle.com>]

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@oracle.com>
> Reviewed-by: Eugene Loh <eugene.loh@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)

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@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel