[DTrace-devel] [PATCH 10/14] Consolidate more tvar and assoc code
Kris Van Hees
kris.van.hees at oracle.com
Tue May 23 19:38:26 UTC 2023
On Tue, May 23, 2023 at 02:44:21PM -0400, Eugene Loh via DTrace-devel wrote:
> 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?
Yes, use:
if (idp->di_kind == DT_IDENT_ARRAY || (idp->di_flags & DT_IDFLG_TLS)) {
dt_cg_prep_dvar_args(dst, dlp, drp, idp, 0, &fnp);
...
}
And perhaps you could add a DT_IDENT_IS_DVAR(idp) macro to capture the entire
conditional, to make it very claer what we are testing for.
Either way, having a conditional and then calling dt_cg_prep_dvar_args() in the
code block is similar to the rest of the dt_cg_load_var() and dt_cg_store_var()
functions, where they check for a variable kind, and then have a code block to
handle it.
The dt_cg_prep_dvar_args() function can then assume it is called for a dvar,
and use the conditionals already there to handle the two specific cases. (And
in the future, perhaps we can combine them in a single call to BPF code, and
do the handling of two distinct cases there - not sure yet if that would be
better.)
> 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;
Why is this still at the top level? It is only used in a code block below.
> > > + 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);
This is the code block that uses lbl_done (and it is the only place it is
used so it should be declared in this block.
More information about the DTrace-devel
mailing list