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

[Eugene Loh <eugene.loh@oracle.com>]

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.

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/

> +				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?

>   	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.)

> -	&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,