[DTrace-devel] [PATCH 5/9] Handle stack() and ustack() args in separate functions

[Kris Van Hees]

See my comments for patch [7/9] om the series... I do not think there is a
need (or benefit) to separate the argument handling for stack and ustack into
separate functions.

On Thu, Oct 05, 2023 at 05:14:03PM -0400, eugene.loh at oracle.com wrote:
> 
> Currently, dt_cg_act_stack() and dt_cg_act_ustack() generate BPF code
> to implement the stack() and ustack() actions.  However, it should also
> be possible to use stack() and ustack() as aggregation keys.  So, it
> would be nice to move the main work of the code-generation functions
> to separate functions that can be used by different callers.
> 
> Therefore, start refactoring these cg functions so that what they do
> can be invoked by multiple callers.  Here, simply move the handling of
> the stack() and ustack() args out into separate functions.  (Actually,
> that handling does not even involve any code generation.)
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  libdtrace/dt_cg.c | 61 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index cef41e6a..389e4dba 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2163,17 +2163,11 @@ dt_cg_act_speculate(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	dt_regset_free(drp, dnp->dn_reg);
>  }
>  
> -static void
> -dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +static int
> +dt_cg_stack_nframes(dtrace_hdl_t *dtp, dt_node_t *dnp)
>  {
> -	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> -	dt_irlist_t	*dlp = &pcb->pcb_ir;
> -	dt_regset_t	*drp = pcb->pcb_regs;
>  	dt_node_t	*arg = dnp->dn_args;
>  	int		nframes = dtp->dt_options[DTRACEOPT_STACKFRAMES];
> -	int		skip = 0;
> -	uint_t		off;
> -	uint_t		lbl_valid = dt_irlist_label(dlp);
>  
>  	if (nframes == DTRACEOPT_UNSET)
>  		nframes = _dtrace_stackframes;
> @@ -2192,6 +2186,22 @@ dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	if (nframes > dtp->dt_options[DTRACEOPT_MAXFRAMES])
>  		nframes = dtp->dt_options[DTRACEOPT_MAXFRAMES];
>  
> +	return nframes;
> +}
> +
> +static void
> +dt_cg_act_stack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +{
> +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> +	dt_irlist_t	*dlp = &pcb->pcb_ir;
> +	dt_regset_t	*drp = pcb->pcb_regs;
> +	int		nframes;
> +	int		skip = 0;
> +	uint_t		off;
> +	uint_t		lbl_valid = dt_irlist_label(dlp);
> +
> +	nframes = dt_cg_stack_nframes(dtp, dnp);
> +
>  	/* Reserve space in the output buffer. */
>  	off = dt_rec_add(dtp, dt_cg_fill_gap, DTRACEACT_STACK,
>  			 sizeof(uint64_t) * nframes, sizeof(uint64_t),
> @@ -2368,20 +2378,13 @@ dt_cg_act_trunc(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	dt_cg_store_val(pcb, trunc, DTRACEACT_LIBACT, NULL, DT_ACT_TRUNC);
>  }
>  
> -static void
> -dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +static uint64_t
> +dt_cg_ustack_arg(dtrace_hdl_t *dtp, dt_node_t *dnp)
>  {
> -	dt_irlist_t	*dlp = &pcb->pcb_ir;
> -	dt_regset_t	*drp = pcb->pcb_regs;
> -	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
>  	int		nframes = dtp->dt_options[DTRACEOPT_USTACKFRAMES];
>  	int		strsize = 0;
> -	int		stacksize;
>  	dt_node_t	*arg0 = dnp->dn_args;
>  	dt_node_t	*arg1 = arg0 != NULL ? arg0->dn_list : NULL;
> -	int		skip = 0;
> -	uint_t		off;
> -	uint_t		lbl_valid = dt_irlist_label(dlp);
>  
>  	if (nframes == DTRACEOPT_UNSET)
>  		nframes = _dtrace_ustackframes;
> @@ -2398,7 +2401,6 @@ dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  	if (nframes > dtp->dt_options[DTRACEOPT_MAXFRAMES])
>  		nframes = dtp->dt_options[DTRACEOPT_MAXFRAMES];
>  
> -	/* FIXME: for now, accept non-zero strsize, but it does nothing */
>  	if (arg1 != NULL) {
>  		if (arg1->dn_kind != DT_NODE_INT ||
>  		    ((arg1->dn_flags & DT_NF_SIGNED) &&
> @@ -2409,11 +2411,28 @@ dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>  		strsize = arg1->dn_value;
>  	}
>  
> -	/* Reserve space in the output buffer. */
> +	return DTRACE_USTACK_ARG(nframes, strsize);
> +}
> +
> +static void
> +dt_cg_act_ustack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> +{
> +	dt_irlist_t	*dlp = &pcb->pcb_ir;
> +	dt_regset_t	*drp = pcb->pcb_regs;
> +	dtrace_hdl_t	*dtp = pcb->pcb_hdl;
> +	uint64_t	arg;
> +	int		nframes, stacksize;
> +	int		skip = 0;
> +	uint_t		off;
> +	uint_t		lbl_valid = dt_irlist_label(dlp);
> +
> +	arg = dt_cg_ustack_arg(dtp, dnp);
> +	nframes = DTRACE_USTACK_NFRAMES(arg);
>  	stacksize = nframes * sizeof(uint64_t);
> +
> +	/* Reserve space in the output buffer. */
>  	off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, DTRACEACT_USTACK,
> -			 8 + stacksize, 8, NULL,
> -			 DTRACE_USTACK_ARG(nframes, strsize));
> +			 8 + stacksize, 8, NULL, arg);
>  
>  	if (dt_regset_xalloc_args(drp) == -1)
>  		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> -- 
> 2.18.4
> 
>