[DTrace-devel] [PATCH 10/14] Consolidate more tvar and assoc code
Eugene Loh
eugene.loh at oracle.com
Tue May 23 18:44:21 UTC 2023
On 5/23/23 13:23, Kris Van Hees wrote:
> On Mon, May 01, 2023 at 11:47:18PM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>> libdtrace/dt_cg.c | 95 ++++++++++++++++-------------------------------
>> 1 file changed, 31 insertions(+), 64 deletions(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 55abcecf..7b98af5c 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -2504,57 +2504,58 @@ static void
>> dt_cg_arglist(dt_ident_t *idp, dt_node_t *args, dt_irlist_t *dlp,
>> dt_regset_t *drp);
>>
>> -static void
>> -dt_cg_load_var(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
>> +static int
>> +dt_cg_prep_dvar_args(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>> + dt_ident_t *idp, int isstore, dt_ident_t **fnpp)
>> {
>> - dt_ident_t *idp = dt_ident_resolve(dst->dn_ident);
>> - dt_ident_t *fnp;
>> - int args_are_ready = 0;
> Not needed - see below.
>
>> -
>> - idp->di_flags |= DT_IDFLG_DIFR;
>> + int args_are_ready = 0;
Ah, I guess here is where you meant "not needed." Yeah, I guess you're
right. That was vestigial. Okay, fixed. Thanks.
>> + uint_t varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>>
>> - if (dst->dn_ident->di_kind == DT_IDENT_ARRAY) {
>> + if (idp->di_kind == DT_IDENT_ARRAY) {
>> /* associative arrays */
>> - uint_t varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>> -
>> - fnp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_assoc");
>> + dt_node_t *args = isstore ? dnp->dn_left : dnp;
>> + *fnpp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_assoc");
>>
>> - assert(dst->dn_kind == DT_NODE_VAR);
>> - assert(!(dst->dn_ident->di_flags & DT_IDFLG_LOCAL));
>> - assert(dst->dn_args != NULL);
>> -
>> - /* Get the tuple. */
>> - dt_cg_arglist(dst->dn_ident, dst->dn_args, dlp, drp);
>> + dt_cg_arglist(idp, args->dn_args, dlp, drp);
>>
>> if (dt_regset_xalloc_args(drp) == -1)
>> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>>
>> emit(dlp, BPF_MOV_IMM(BPF_REG_1, varid));
>> - emit(dlp, BPF_MOV_REG(BPF_REG_2, dst->dn_args->dn_reg));
>> - dt_regset_free(drp, dst->dn_args->dn_reg);
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_3, 0));
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_4, 0));
>> + emit(dlp, BPF_MOV_REG(BPF_REG_2, args->dn_args->dn_reg));
>> + dt_regset_free(drp, args->dn_args->dn_reg);
>> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, isstore));
>> + emit(dlp, BPF_MOV_REG(BPF_REG_4, isstore ? dnp->dn_reg : 0));
>> dt_cg_zerosptr(BPF_REG_5, dlp, drp);
>>
>> args_are_ready = 1;
> All you ever do with args_are_ready is either keep it to the default 0 or
> assign 1 to it, and then return its value. You can just return 1 here.
Yip. Thanks.
>> } else if (idp->di_flags & DT_IDFLG_TLS) {
>> /* thread-local variables */
>> - uint_t varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>> -
>> - fnp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_tvar");
>> + *fnpp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_tvar");
>>
>> if (dt_regset_xalloc_args(drp) == -1)
>> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>>
>> emit(dlp, BPF_MOV_IMM(BPF_REG_1, varid));
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_2, 0));
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_3, 0));
>> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, isstore));
>> + emit(dlp, BPF_MOV_REG(BPF_REG_3, isstore ? dnp->dn_reg : 0));
>> dt_cg_zerosptr(BPF_REG_4, dlp, drp);
>>
>> args_are_ready = 1;
> Just return 1.
Ditto.
>> }
>>
>> - if (args_are_ready) {
>> + return args_are_ready;
> Just return 0.
Ditto.
>> +}
>> +
>> +static void
>> +dt_cg_load_var(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
>> +{
>> + dt_ident_t *idp = dt_ident_resolve(dst->dn_ident);
>> + dt_ident_t *fnp;
>> +
>> + idp->di_flags |= DT_IDFLG_DIFR;
>> +
>> + if (dt_cg_prep_dvar_args(dst, dlp, drp, idp, 0, &fnp)) {
> This looks weird to me... And confusing. Surely we don't care to call
> anything that involves dvars or dvar_args for variables that are not dynamic
> variables. I think it would make sense to have a conditional here so that
> we only call dt_cg_prep_dvar_args() when needed.
>
> (Same for dt_cg_store_var().)
>
> Yes, it will add one extra branch but I think it is worth it to make the
> code cleaner/clearer.
I see what you're saying, but I also see it the other way. The idea was
to call prep_dvar(). As you say, that doesn't always make sense, but
that's why we check the return value. The function can refuse: "Sorry,
no can do!" In my opinion, this way is cleaner. The alternative is to
move these checks up to both call sites. Then, prep_dvar() can, I dunno,
assume it's a dvar or double check again (producing an error since it's
just second guessing the caller) or whatever. Maybe your concern can be
addressed by changing prep_dvar()'s name? Or a comment for the function?
In short, I see what you mean, but I think it's cleaner as is.
Nevertheless, if you really want the call sites to make these checks,
just let me know and I'll make the change.
>> assert(fnp != NULL);
>>
>> dt_regset_xalloc(drp, BPF_REG_0);
>> @@ -3155,8 +3156,8 @@ static void
>> dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>> dt_ident_t *idp)
>> {
>> - uint_t varid, lbl_done;
>> - int reg, args_are_ready = 0;
>> + uint_t lbl_done;
>> + int reg;
>> size_t size;
>> dt_ident_t *fnp;
>>
>> @@ -3177,41 +3178,7 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>> idp->di_name);
>> }
>>
>> - if (idp->di_kind == DT_IDENT_ARRAY) {
>> - /* Associative (global or TLS) array. Cannot be in alloca space. */
>> - varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>> - fnp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_assoc");
>> -
>> - dt_cg_arglist(idp, dnp->dn_left->dn_args, dlp, drp);
>> -
>> - if (dt_regset_xalloc_args(drp) == -1)
>> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> -
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_1, varid));
>> - emit(dlp, BPF_MOV_REG(BPF_REG_2, dnp->dn_left->dn_args->dn_reg));
>> - dt_regset_free(drp, dnp->dn_left->dn_args->dn_reg);
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_3, 1));
>> - emit(dlp, BPF_MOV_REG(BPF_REG_4, dnp->dn_reg));
>> - dt_cg_zerosptr(BPF_REG_5, dlp, drp);
>> -
>> - args_are_ready = 1;
>> - } else if (idp->di_flags & DT_IDFLG_TLS) {
>> - /* TLS var */
>> - varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>> - fnp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_tvar");
>> -
>> - if (dt_regset_xalloc_args(drp) == -1)
>> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>> -
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_1, varid));
>> - emit(dlp, BPF_MOV_IMM(BPF_REG_2, 1));
>> - emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
>> - dt_cg_zerosptr(BPF_REG_4, dlp, drp);
>> -
>> - args_are_ready = 1;
>> - }
>> -
>> - if (args_are_ready) {
>> + if (dt_cg_prep_dvar_args(dnp, dlp, drp, idp, 1, &fnp)) {
>> assert(fnp != NULL);
>>
>> dt_regset_xalloc(drp, BPF_REG_0);
>> --
>> 2.18.4
>>
>>
>> _______________________________________________
>> 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