[DTrace-devel] [PATCH v2] Add support for uregs[]

[Eugene Loh]

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