Re: [PATCH 2/2] cg: transition [u]stack() from action to subroutine

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

On Mon, Sep 29, 2025 at 12:22:16AM -0400, Eugene Loh wrote:
> I don't follow all the details, but I guess this looks fine to me.  I did
> have a few minor syntactical comments.
> 
> It seems that dt_cg_subr_stack() and dt_cg_subr_ustack() are virtually
> identical.  Might one combine the two functions into a
> dt_cg_subr_stack_common() (or whatever name seems suitable) and do this:
> 
> static void
> dt_cg_subr_stack(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> {
>         dt_cg_subr_stack_common(dnp, dlp, drp, DTRACEACT_STACK);
> }
> 
> static void
> dt_cg_subr_ustack(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> {
>         dt_cg_subr_stack_common(dnp, dlp, drp, DTRACEACT_USTACK);
> }
> 
> The win is less the reduction in lines of code and more in the clarity of
> what is being done.

Sure.

> Also, a few others sprinkled below...
> 
> On 9/23/25 12:01, Kris Van Hees wrote:
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -3987,50 +3996,63 @@ empty_args:
> >   		uint_t			nextoff;
> >   		int			is_symmod = 0;
> > -		if (dnp->dn_kind == DT_NODE_FUNC &&
> > -		    dnp->dn_ident != NULL)
> > -			switch (dnp->dn_ident->di_id) {
> > -			case DT_ACT_STACK:
> > -				tuplesize = dt_cg_act_stack_sub(yypcb, dnp, treg, tuplesize, DTRACEACT_STACK);
> > -				continue;
> > -			case DT_ACT_USTACK:
> > -				tuplesize = dt_cg_act_stack_sub(yypcb, dnp, treg, tuplesize, DTRACEACT_USTACK);
> > -				continue;
> > -			case DT_ACT_UADDR:
> > -			case DT_ACT_USYM:
> > -			case DT_ACT_UMOD:
> > -				nextoff = (tuplesize + (8 - 1)) & ~(8 - 1);
> > -				if (tuplesize < nextoff)
> > -					emit(dlp,  BPF_ALU64_IMM(BPF_ADD, treg, nextoff - tuplesize));
> > -
> > -				/* Preface the value with the user process pid. */
> > -				if (dt_regset_xalloc_args(drp) == -1)
> > -					longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > -				dt_regset_xalloc(drp, BPF_REG_0);
> > -				emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> > -				dt_regset_free_args(drp);
> > -				emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
> > -				emit(dlp, BPF_STORE(BPF_DW, treg, 0, BPF_REG_0));
> > -				dt_regset_free(drp, BPF_REG_0);
> > -
> > -				/* Then store the value. */
> > -				dt_regset_xalloc(drp, BPF_REG_0);
> > -				emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, areg, -i * DT_STK_SLOT_SZ));
> > -				emit(dlp,  BPF_STORE(BPF_DW, treg, 8, BPF_REG_0));
> > -				dt_regset_free(drp, BPF_REG_0);
> > -
> > -				emit(dlp,  BPF_ALU64_IMM(BPF_ADD, treg, 16));
> > -				tuplesize = nextoff + 16;
> > -
> > -				continue;
> > -			case DT_ACT_SYM:
> > -			case DT_ACT_MOD:
> > -				is_symmod = 1;
> > -				size = 8;
> > -				dt_regset_xalloc(drp, BPF_REG_0);
> > -				emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, areg, -i * DT_STK_SLOT_SZ));
> > -				break;
> > +		if (dnp->dn_kind == DT_NODE_FUNC) {
> > +			dt_ident_t	*fidp = dnp->dn_ident;
> > +
> > +			if (fidp->di_kind == DT_IDENT_FUNC) {
> > +				switch (fidp->di_id) {
> > +				case DIF_SUBR_STACK:
> > +					tuplesize = dt_cg_act_stack_sub(
> > +							yypcb, dnp, treg,
> > +							tuplesize,
> > +							DTRACEACT_STACK);
> > +					continue;
> > +				case DIF_SUBR_USTACK:
> > +					tuplesize = dt_cg_act_stack_sub(
> > +							yypcb, dnp, treg,
> > +							tuplesize,
> > +							DTRACEACT_USTACK);
> > +					continue;
> > +				}
> > +			} else if (fidp->di_kind == DT_IDENT_ACTFUNC) {
> > +				switch (dnp->dn_ident->di_id) {
> 
> s/dnp->dn_ident/fidp/

Thanks.

> > +				case DT_ACT_UADDR:
> > +				case DT_ACT_USYM:
> > +				case DT_ACT_UMOD:
> > +					nextoff = ALIGN(tuplesize, 8);
> > +					if (tuplesize < nextoff)
> > +						emit(dlp,  BPF_ALU64_IMM(BPF_ADD, treg, nextoff - tuplesize));
> > +
> > +					/* Preface the value with the user process pid. */
> > +					if (dt_regset_xalloc_args(drp) == -1)
> > +						longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > +					dt_regset_xalloc(drp, BPF_REG_0);
> > +					emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> > +					dt_regset_free_args(drp);
> > +					emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
> > +					emit(dlp, BPF_STORE(BPF_DW, treg, 0, BPF_REG_0));
> > +					dt_regset_free(drp, BPF_REG_0);
> > +
> > +					/* Then store the value. */
> > +					dt_regset_xalloc(drp, BPF_REG_0);
> > +					emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, areg, -i * DT_STK_SLOT_SZ));
> > +					emit(dlp,  BPF_STORE(BPF_DW, treg, 8, BPF_REG_0));
> > +					dt_regset_free(drp, BPF_REG_0);
> > +
> > +					emit(dlp,  BPF_ALU64_IMM(BPF_ADD, treg, 16));
> > +					tuplesize = nextoff + 16;
> > +
> > +					continue;
> > +				case DT_ACT_SYM:
> > +				case DT_ACT_MOD:
> > +					is_symmod = 1;
> > +					size = 8;
> > +					dt_regset_xalloc(drp, BPF_REG_0);
> > +					emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, areg, -i * DT_STK_SLOT_SZ));
> > +					break;
> > +				}
> >   			}
> > +		}
> >   		if (!is_symmod) {
> >   			dt_node_diftype(dtp, dnp, &t);
> > @@ -6968,6 +7070,7 @@ dt_cg_call_subr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >   	assert(idp->di_id <= DIF_SUBR_MAX);
> >   	fun = _dt_cg_subr[idp->di_id];
> > +assert(fun != NULL);
> 
> Weird indentation?

I think I added that as debugging.  Will remove.

> >   	if (fun == NULL)
> >   		dnerror(dnp, D_FUNC_UNDEF, "unimplemented subroutine: %s\n",
> >   			idp->di_name);
> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index 509f263d..9dd8956d 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -175,7 +175,7 @@ static const dt_ident_t _dtrace_globals[] = {
> >   { "ipl", DT_IDENT_SCALAR, 0, DIF_VAR_IPL, DT_ATTR_STABCMN, DT_VERS_1_0,
> >   	&dt_idops_type, "uint_t" },
> >   { "jstack", DT_IDENT_ACTFUNC, 0, DT_ACT_JSTACK, DT_ATTR_STABCMN, DT_VERS_1_0,
> 
> Add DT_IDFLG_ALLOCA?  (Of course, it doesn't matter yet.)

Ah no, some other changes will make that not even needed because I will be
using the static area for storing stacks rather than allocating a new one for
each invocation.

> > -	&dt_idops_func, "stack([uint32_t], [uint32_t])" },
> > +	&dt_idops_func, "dt_stack([uint32_t], [uint32_t])" },
> >   { "link_ntop", DT_IDENT_FUNC, 0, DIF_SUBR_LINK_NTOP, DT_ATTR_STABCMN,
> >   	DT_VERS_1_5, &dt_idops_func, "string(int, void *)" },
> >   { "llquantize", DT_IDENT_AGGFUNC, 0, DT_AGG_LLQUANTIZE,