[DTrace-devel] [PATCH v2] Add support for uregs[]
Eugene Loh
eugene.loh at oracle.com
Fri Feb 24 20:42:15 UTC 2023
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).
>> + 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[]).
>> + 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)?
>> +
>> + 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.
>> 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));
More information about the DTrace-devel
mailing list