[DTrace-devel] [PATCH 2/3] Combine assertion with conversion of size to BPF width
Kris Van Hees
kris.van.hees at oracle.com
Fri Sep 1 18:19:15 UTC 2023
I already gave a review for this but I actually posted an alternative patch to
do the same thing (using the option to report an internal compiler error if
a store or load with the wrong size is used). Sicne this is always a problem
with our code (assert or internal error reported) the user experience will not
change in any noticable way - and if it does, reporting an error is still
better than an assert triggering).
It also involved introducing 3 new mzcros to make it easier to write code to
use this without needing to do explicit calls etc.
On Tue, Aug 22, 2023 at 05:46:55PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> In dt_cg.c, we often convert a size 1, 2, 4, or 8 into a BPF width
> BPF_B, BPF_H, BPF_W, or BPF_DW, respectively. Each time, we first
> check that the size satisfies
> assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
> and then we look up ldstw[size].
>
> Combine the assertion and the conversion to simplify the multiple
> call sites.
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 83 +++++++++++++++--------------------------------
> 1 file changed, 26 insertions(+), 57 deletions(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 0a7149b3..ea3745bc 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1267,11 +1267,16 @@ dt_cg_check_ptr_arg(dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dnp,
> dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
> }
>
> -static const uint_t ldstw[] = {
> - 0,
> - BPF_B, BPF_H, 0, BPF_W,
> - 0, 0, 0, BPF_DW,
> - };
> +static int BPF_width(size_t sz)
> +{
> + switch(sz) {
> + case 1: return BPF_B;
> + case 2: return BPF_H;
> + case 4: return BPF_W;
> + case 8: return BPF_DW;
> + default: assert(0);
> + }
> +}
>
> /*
> * When loading bit-fields, we want to convert a byte count in the range
> @@ -1364,9 +1369,7 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, size, pfp,
> arg);
>
> - assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
> -
> - emit(dlp, BPF_STORE(ldstw[size], BPF_REG_9, off, dnp->dn_reg));
> + emit(dlp, BPF_STORE(BPF_width(size), BPF_REG_9, off, dnp->dn_reg));
> dt_regset_free(drp, dnp->dn_reg);
>
> goto ok;
> @@ -2558,15 +2561,10 @@ dt_cg_ldsize(dt_node_t *dnp, ctf_file_t *ctfp, ctf_id_t type, ssize_t *ret_size)
> else
> size = ctf_type_size(ctfp, type);
>
> - if (size < 1 || size > 8 || (size & (size - 1)) != 0) {
> - xyerror(D_UNKNOWN, "internal error -- cg cannot load "
> - "size %ld when passed by value\n", (long)size);
> - }
> -
> if (ret_size)
> *ret_size = size;
>
> - return ldstw[size];
> + return BPF_width(size);
> }
>
> /*
> @@ -2714,14 +2712,11 @@ dt_cg_load_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> uint_t lbl_notnull = dt_irlist_label(dlp);
> uint_t lbl_done = dt_irlist_label(dlp);
>
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
> -
> emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, 0, lbl_notnull));
> emit(dlp, BPF_MOV_IMM(dnp->dn_reg, 0));
> emit(dlp, BPF_JUMP(lbl_done));
> emitl(dlp, lbl_notnull,
> - BPF_LOAD(ldstw[size], dnp->dn_reg, BPF_REG_0, 0));
> + BPF_LOAD(BPF_width(size), dnp->dn_reg, BPF_REG_0, 0));
> dt_cg_promote(dnp, size, dlp, drp);
>
> emitl(dlp, lbl_done,
> @@ -2758,12 +2753,10 @@ dt_cg_load_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> uint_t L2 = dt_irlist_label(dlp);
>
> assert(dnp->dn_flags & DT_NF_REF);
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
>
> if ((reg = dt_regset_alloc(drp)) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> - emit(dlp, BPF_LOAD(ldstw[size], reg, dnp->dn_reg, idp->di_offset));
> + emit(dlp, BPF_LOAD(BPF_width(size), reg, dnp->dn_reg, idp->di_offset));
> emit(dlp, BPF_BRANCH_IMM(BPF_JNE, reg, DT_NULL_STRING, L1));
> emit(dlp, BPF_MOV_IMM(dnp->dn_reg, 0));
> emit(dlp, BPF_JUMP(L2));
> @@ -2778,10 +2771,7 @@ dt_cg_load_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> } else {
> size_t size = dt_node_type_size(dnp);
>
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
> -
> - emit(dlp, BPF_LOAD(ldstw[size], dnp->dn_reg, dnp->dn_reg, idp->di_offset));
> + emit(dlp, BPF_LOAD(BPF_width(size), dnp->dn_reg, dnp->dn_reg, idp->di_offset));
> dt_cg_promote(dnp, size, dlp, drp);
> }
>
> @@ -3029,9 +3019,7 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
> else
> sreg = src->dn_reg;
>
> - assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
> -
> - emit(dlp, BPF_STORE(ldstw[size], dreg, 0, sreg));
> + emit(dlp, BPF_STORE(BPF_width(size), dreg, 0, sreg));
>
> if (sreg != src->dn_reg)
> dt_regset_free(drp, sreg);
> @@ -3211,14 +3199,11 @@ empty_args:
> size = dt_node_sizeof(&isp->dis_args[i]);
>
> if (dt_node_is_scalar(dnp) || dt_node_is_float(dnp)) {
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
> -
> nextoff = (tuplesize + (size - 1)) & ~(size - 1);
> if (tuplesize < nextoff)
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, nextoff - tuplesize));
>
> - emit(dlp, BPF_STORE(ldstw[size], treg, 0, BPF_REG_0));
> + emit(dlp, BPF_STORE(BPF_width(size), treg, 0, BPF_REG_0));
> dt_regset_free(drp, BPF_REG_0);
>
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, size));
> @@ -3381,9 +3366,7 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>
> dt_cg_memcpy(dlp, drp, reg, dnp->dn_reg, size);
> } else {
> - assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
> -
> - emit(dlp, BPF_STORE(ldstw[size], reg, 0, dnp->dn_reg));
> + emit(dlp, BPF_STORE(BPF_width(size), reg, 0, dnp->dn_reg));
> }
>
> dt_regset_free(drp, reg);
> @@ -3425,11 +3408,9 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> assert(idp->di_size == dt_node_type_size(dnp->dn_left));
> assert(dt_node_is_integer(dnp->dn_right) == (dnp->dn_right->dn_kind == DT_NODE_INT));
> assert(dnp->dn_right->dn_value == 0);
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
>
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
> - emit(dlp, BPF_STORE_IMM(ldstw[size], reg, 0, DT_NULL_STRING));
> + emit(dlp, BPF_STORE_IMM(BPF_width(size), reg, 0, DT_NULL_STRING));
>
> /*
> * Since strings are passed by value, we need to force
> @@ -3461,10 +3442,8 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>
> /* Start by zeroing out the first bytes */
> size = sizeof(DT_NULL_STRING);
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
>
> - emit(dlp, BPF_STORE_IMM(ldstw[size], reg, 0, 0));
> + emit(dlp, BPF_STORE_IMM(BPF_width(size), reg, 0, 0));
>
> /*
> * Determine the amount of data to be copied. It is
> @@ -3485,8 +3464,7 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> emitl(dlp, Lnull,
> BPF_NOP());
> size = sizeof(DT_NULL_STRING);
> - assert(size > 0 && size <= 8 && (size & (size - 1)) == 0);
> - emit(dlp, BPF_STORE_IMM(ldstw[size], reg, 0, DT_NULL_STRING));
> + emit(dlp, BPF_STORE_IMM(BPF_width(size), reg, 0, DT_NULL_STRING));
> emitl(dlp, Ldone,
> BPF_NOP());
> }
> @@ -3521,10 +3499,7 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> /* Store by value */
> size = idp->di_size;
>
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
> -
> - emit(dlp, BPF_STORE(ldstw[size], reg, idp->di_offset, dnp->dn_reg));
> + emit(dlp, BPF_STORE(BPF_width(size), reg, idp->di_offset, dnp->dn_reg));
> dt_regset_free(drp, reg);
> }
>
> @@ -3671,9 +3646,6 @@ dt_cg_incdec_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op,
>
> TRACE_REGSET(" incdec: Begin");
>
> - assert(size > 0 && size <= 8 &&
> - (size & (size - 1)) == 0);
> -
> if (dt_node_is_pointer(dnp)) {
> ctf_file_t *ctfp = dnp->dn_ctfp;
> ctf_id_t type;
> @@ -3737,16 +3709,16 @@ dt_cg_incdec_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp, uint_t op,
> BPF_NOP());
> }
>
> - emit(dlp, BPF_LOAD(ldstw[size], dnp->dn_reg, dnp->dn_child->dn_reg, 0));
> + emit(dlp, BPF_LOAD(BPF_width(size), dnp->dn_reg, dnp->dn_child->dn_reg, 0));
> if (post) {
> dt_regset_xalloc(drp, BPF_REG_0);
> emit(dlp, BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
> emit(dlp, BPF_ALU64_IMM(op, BPF_REG_0, adj));
> - emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0, BPF_REG_0));
> + emit(dlp, BPF_STORE(BPF_width(size), dnp->dn_child->dn_reg, 0, BPF_REG_0));
> dt_regset_free(drp, BPF_REG_0);
> } else {
> emit(dlp, BPF_ALU64_IMM(op, dnp->dn_reg, adj));
> - emit(dlp, BPF_STORE(ldstw[size], dnp->dn_child->dn_reg, 0, dnp->dn_reg));
> + emit(dlp, BPF_STORE(BPF_width(size), dnp->dn_child->dn_reg, 0, dnp->dn_reg));
> }
>
> dt_regset_free(drp, dnp->dn_child->dn_reg);
> @@ -4261,9 +4233,6 @@ dt_cg_uregs(unsigned int idx, dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp
> /* Add the thread->member offset. */
> offset += dt_cg_ctf_offsetof("struct thread_struct",
> memnames[idx - 21], &size);
> - if (size < 1 || size > 8 || (size & (size - 1)) != 0)
> - xyerror(D_UNKNOWN, "internal error -- cg cannot load "
> - "size %ld when passed by value\n", (long)size);
>
> /* Get task. */
> if (dt_regset_xalloc_args(drp) == -1)
> @@ -4278,7 +4247,7 @@ dt_cg_uregs(unsigned int idx, dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, offset));
>
> /* Dereference it safely (the BPF verifier has no idea what it is). */
> - dt_cg_load_scalar(dnp, ldstw[size], size, dlp, drp);
> + dt_cg_load_scalar(dnp, BPF_width(size), size, dlp, drp);
>
> return;
> }
> --
> 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