Re: [PATCH 2/2] cg: transition [u]stack() from action to subroutine
Kris Van Hees <[email protected]>
| Newsgroups | dev.linux.lists.dtrace |
|---|---|
| Message-ID | <[email protected]> |
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,