[DTrace-devel] [PATCH 2/3] Combine assertion with conversion of size to BPF width
Kris Van Hees
kris.van.hees at oracle.com
Wed Aug 23 03:37:56 UTC 2023
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.
There are a couple of issues with this. THe biggest is information loss by
replacing a semi-informative assert with an assert(0). And in two cases, a
compiler error is being replaced by the assert(0) which certainly causes a
loss of information (and a change in behviour).
> 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)
I really would like us to stay away from function names with all caps elements.
Typically, we use that for macros. All other BPF-related functions use 'bpf'
so at a minimum, so bpf_width() would be preferred. But in all, why not make
this ldstw(size_t sz) so it looks almost the same to what we had? And it makes
it nice and compact.
> +{
> + 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);
This assert(0) becomes a problem because it hides the reason why we flag an
assert in the first place. You need to go lookup the source code to get any
idea about what might be wrong - that is a step back from what we had.
I can see two options here:
1. Make the assert be what it was before. That does add code that is simply
evaluating to a truth value that we already know, but that is OK because it
is only executed when the assert will fire anyway.
2. Turn the assert into triggering a compilation error. There are already two
places where an error is flagged rather than an assert getting triggered so
it is not out of the ordinary. And while this assert (in most case) guards
against internal errors in our code, using a compiler error for that will
yield the same result, even if it is not entirely the same as the legacy
code. But if we're going to deviate anyway, why not make this more useful.
And again, there is no user-noticable change here because if there was,
people would be running into these asserts (and they are not - thankfully).
I definitely prefer option 2 (which would probably require passing in a flag to
say whether this is a load or store).
> + }
> +}
>
> /*
> * 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