[DTrace-devel] [PATCH v2] Add support for uregs[]
Kris Van Hees
kris.van.hees at oracle.com
Fri Feb 24 21:25:18 UTC 2023
On Fri, Feb 24, 2023 at 03:42:15PM -0500, Eugene Loh via DTrace-devel wrote:
> I'll post a v3 momentarily, but...
>
> On 2/24/23 14:12, Kris Van Hees wrote:
> > On Tue, Feb 21, 2023 at 11:49:11AM -0500, eugene.loh--- via DTrace-devel wrote:
> > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > +
> > > +static void
> > > +dt_cg_uregs(unsigned int idx, dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > > +{
> > > + int regmap[] = { DT_CG_REG_RBX, /* 0 -> EBX */
> > > + DT_CG_REG_RCX, /* 1 -> ECX */
> > > + DT_CG_REG_RDX, /* 2 -> EDX */
> > > + DT_CG_REG_RSI, /* 3 -> ESI */
> > > + DT_CG_REG_RDI, /* 4 -> EDI */
> > > + DT_CG_REG_RBP, /* 5 -> EBP */
> > > + DT_CG_REG_RAX, /* 6 -> EAX */
> > > + DT_CG_REG_DS, /* 7 -> DS */
> > > + DT_CG_REG_ES, /* 8 -> ES */
> > > + DT_CG_REG_FS, /* 9 -> FS */
> > > + DT_CG_REG_GS, /* 10 -> GS */
> > > + DT_CG_REG_TRAPNO, /* 11 -> TRAPNO */
> > > + DT_CG_REG_RIP, /* 12 -> EIP */
> > > + DT_CG_REG_CS, /* 13 -> CS */
> > > + DT_CG_REG_RFL, /* 14 -> EFL */
> > > + DT_CG_REG_RSP, /* 15 -> UESP */
> > > + DT_CG_REG_SS, /* 16 -> SS */
> > > + };
> > > + dtrace_hdl_t *dtp = yypcb->pcb_hdl;
> > > + ctf_file_t *cfp = dtp->dt_shared_ctf;
> > > + ctf_id_t type;
> > > + ctf_membinfo_t ctm;
> > > + int offset, rc;
> > > +
> > > + if (idx >= sizeof(regmap) / sizeof(int) + DT_CG_REG_GS + 1) {
> > > + dnerror(dnp, D_UNKNOWN, "uregs[%d]: unrecognized index\n", idx);
> > I think a better message would be "uregs[]: index out of bounds (%d)\n" (also
> > more consistent).
> I made the change, but "out of bounds" is a little strange since there are
> presumably no bounds... just symbolic values (as though into an associative
> array).
The uregs[] builtin is described in the documentation as an array. And the way
it is implemented (and used) definitely support the notion that it is used as a
regular array with integer index. After all, otherwise it would be a lot
easier to just index it by the name of the register, avoiding any form of
index mapping altogether.
> > > + return;
> > > + }
> > > +
> > > + /* convert register alias index into register mapping index */
> > > + if (idx > DT_CG_REG_TRAPNO)
> > > + idx = regmap[idx - DT_CG_REG_GS - 1];
> > > +
> > > + /* look up pt_regs[] */
> > > + if (idx <= DT_CG_REG_SS) {
> > > + if (dtp->dt_bpfhelper[BPF_FUNC_get_current_task_btf] == BPF_FUNC_unspec
> > > + || dtp->dt_bpfhelper[BPF_FUNC_task_pt_regs] == BPF_FUNC_unspec)
> > > + dnerror(dnp, D_UNKNOWN, "uregs[]: pt_regs[] look up is not implemented for this kernel version\n");
> > I think this is exposing more details than the average user cares to know.
> > Just say "uregs[] is not supported on this kernel".
> I made the change, but it'd be weird to see that message for some indices
> but not for others (which don't care about pt_regs[]).
Well, perhaps then it would be better to not support the _GS etc indices either
when the BPF helpers do not exist? That seems more consistent. And honestly,
there is little to no value in support those and not being able to support the
registers that actually matter most.
> > > + 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_task_btf));
> > > + emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_0));
> > > + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_task_pt_regs));
> > > + dt_regset_free_args(drp);
> > > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, idx * sizeof(uint64_t)));
> > > + emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_0, 0));
> > > + dt_regset_free(drp, BPF_REG_0);
> > > + return;
> > > + }
> > > +
> > > + /* look up task->thread offset */
> > > +
> > Extra newline.
> >
> > > + if (!cfp)
> > > + assert(0); // return dt_set_errno(dtp, EDT_NOCTF);
> > Errr.... assert????? And a dt_set_errno in comment?
> The call path is dt_cg_array_ops(), which doesn't check return values. So,
> I cannot return a value to indicate anything. So, what should happen in
> place of assert(0)?
longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> > > +
> > > + 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_task));
> > > + emit(dlp, BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> > > + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, offset));
> > > + emit(dlp, BPF_MOV_IMM(BPF_REG_2, sizeof(uint64_t)));
> > > + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_SP));
> > We usually set up the arguments in ordinal order, if at all possible.
> Yeah, but since our reg spill is not real robust, I do it in this order to
> get around clobbering issues.
> > > @@ -3956,6 +4151,7 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > > dt_ident_t *fidp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_bvar");
> > > size_t size;
> > > int n;
> > > + unsigned int idx;
> > Move this into the code block below for uregs.
> >
> > > assert(dnp->dn_kind == DT_NODE_VAR);
> > > assert(!(idp->di_flags & (DT_IDFLG_TLS | DT_IDFLG_LOCAL)));
> > > @@ -3998,12 +4194,17 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > > }
> > > dt_cg_node(dnp->dn_args, dlp, drp);
> > > + idx = dnp->dn_args->dn_value;
> > Move this into the code block below for uregs. An perhaps add a comment that
> > you can do this because we know the index is a constant integer (DT_NODE_INT).
> Uh... wow, yeah, okay. Makes for cleaner code but I had to think twice
> about why this is okay.
Heh... You added that idx = ... line in the first place. I am merely
suggesting moving it to the only code block that uses it. :)
> > > dnp->dn_args->dn_value = saved;
> > > dnp->dn_reg = dnp->dn_args->dn_reg;
> > > + if (idp->di_id == DIF_VAR_UREGS) {
> > > + dt_cg_uregs(idx, dnp, dlp, drp);
> > > + return;
> > > + }
> > > +
> > > if (dt_regset_xalloc_args(drp) == -1)
> > > longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > > -
> > > emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> > > emit(dlp, BPF_MOV_IMM(BPF_REG_2, idp->di_id));
> > > emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list