[DTrace-devel] [PATCH 10/14] Consolidate more tvar and assoc code
Kris Van Hees
kris.van.hees at oracle.com
Tue May 23 17:23:21 UTC 2023
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;
> + 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.
> } 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.
> }
>
> - if (args_are_ready) {
> + return args_are_ready;
Just return 0.
> +}
> +
> +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.
> 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