[DTrace-devel] [PATCH 06/10 v4] Implement associative array support
Eugene Loh
eugene.loh at oracle.com
Tue Mar 22 01:10:44 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
to expedite review but some comments below.
On 3/18/22 3:04 PM, Kris Van Hees via DTrace-devel wrote:
> Associative arrays (global or TLS) are variables that are indexed with
> a tuple (one or more values). The underlying storage is provided by
> the dvar (dynamic variables) BPF hash map. Since dvar elements are
> indexed using a unique 64-bit ID, and given that it provides storage
> for both regular TLS variables and associate array elements, the
> algorithms to calculate the ID are designed to provide orthogonal
> value ranges.
>
> TLS variable: dvar key has high order bit 0
> Associative array element: dvar key has high order bit 1
>
> The dvar key for an associative array element is determined based on the
> tuple content values. A new BPF hash map (tuples) associates the
> concatenation of the tuple values as key with the address of the hash
> map element (which is guaranteed to be unique). Since BPF maps are
> allocated in kernel space, these addresses will always have high order
> bit 1.
>
> To ensure uniqueness between global associate array elements and TLS
> associate array elements, and to ensure uniqueness between elements
> with the same tuple index in different arrays, the key in the tuples
> map is determined based on a rewritten tuple:
>
> [ variable ID, original tuple values, TLS variable key or 0 ]
>
> While variable IDs are not unique between variable kinds, the final
> component in the rewritten tuple is the TLS variable key (for TLS
> associative arrays - never 0) or 0 (for global associative arrays).
>
> Various new tests are added to exercise the new functionality.
>
> Running out of dynamic variable space is not being reported as a drop
> yet due to lack of drop counter support. It generates an error
> instead, and therefore the test for the drop reporting remains XFAIL.
The hash map is actually "dvars" rather than "dvar".
The commit message strikes me as unclear -- e.g. what is the high-order
bit if something is both TLS *AND* associative? (Answer: 1.) The
description in bpf/get_dvar.c strikes me as somewhat clearer, though
maybe this stuff is just plain complicated. Additionally, it seems like
the message is an amalgamation of different explanations of the
"orthogonality" being targeted.
Here's an attempt at a rewritten commit message:
Implement associative array support
Associative arrays are variables that are indexed with a tuple (one or
more values).
The "dvars" (dynamic variable) BPF hash map provides storage for both
associative and thread-local storage (TLS) data. A 64-bit dvars key
is constructed as follows:
* TLS, non-associative: use the TLS key
* TLS, associative: use
&"tuples"[ varID, original tuple, TLS key ]
* global, associative: use
&"tuples"[ varID, original tuple, 0 ]
Here, "tuples" is a new BPF hash map that gives a unique value address
for each expanded tuple. Since BPF maps are allocated in kernel space,
these addresses will always have high-order bit 1 and cannot coincide
with any TLS keys, whose high-order bit is always 0.
Various new tests are added to exercise the new functionality.
Running out of dynamic variable space is not being reported as a drop
yet due to lack of drop counter support. It generates an error
instead, and therefore the test for the drop reporting remains XFAIL.
> diff --git a/bpf/get_dvar.c b/bpf/get_dvar.c
> index 14f21783..25ce37f4 100644
> --- a/bpf/get_dvar.c
> +++ b/bpf/get_dvar.c
> @@ -11,8 +11,24 @@
> #endif
>
> extern struct bpf_map_def dvars;
> +extern struct bpf_map_def tuples;
> extern uint64_t NCPUS;
>
> +/*
> + * Dynamic variables are identified using a unique 64-bit key. Three diferent
diferent -> different
> + * categories of dynamic variables are supported in DTrace:
> + *
> + * 1. Thread-local storage (TLS) variables:
> + * dvar key = TLS key (highest bit = 0)
> + * 2. Global associative array elements:
> + * dvar key = &tuples[var id, tuple, (uint64_t)0] (highest bit = 1)
> + * 2. TLS associative array elements:
> + * dvar key = &tuples[var id, tuple, TLS key] (highest bit = 1)
1 2 2
->
1 2 3
> + * Given that the TLS key can never be 0, uniqueness of the dvar key is
> + * guaranteed in this scheme.
> + */
> +
> noinline uint64_t dt_tlskey(uint32_t id)
> {
> uint64_t key;
> @@ -25,7 +41,7 @@ noinline uint64_t dt_tlskey(uint32_t id)
> key += (uint32_t)(uint64_t)&NCPUS;
>
> key++;
> - key = (key << 32) | id;
> + key = ((key & 0x7fffffff) << 32) | id;
As I asked in the other email, are we throwing a bit away or are we
simply appeasing the BPF verifier?
> return key;
> }
> @@ -81,3 +97,59 @@ noinline void *dt_get_tvar(uint32_t id, uint64_t store, uint64_t nval)
> {
> return dt_get_dvar(dt_tlskey(id), store, nval);
> }
> +
> +noinline void *dt_get_assoc(uint32_t id, const char *tuple,
> + uint64_t store, uint64_t nval)
> +{
> + uint64_t *valp;
> + uint64_t val;
> + uint64_t dflt_val = 0;
> +
> + /*
> + * Place the variable ID at the beginning of the tuple.
> + */
> + *(uint32_t *)tuple = id;
> +
> + /*
> + * Look for the tuple in the tuples map.
> + */
> + valp = bpf_map_lookup_elem(&tuples, tuple);
> + if (valp == 0) {
> + /*
> + * Not found. If we are not storing a value (i.e. performing a
> + * load), return the default value (0). If we are trying to
> + * delete an associative array element, we don't have to do
> + * anything because it does not exist anyway.
> + */
> + if (!store || !nval)
> + return 0;
> +
> + /*
> + * Create the tuple and use the address of the value as the
> + * actual value.
> + */
> + if (bpf_map_update_elem(&tuples, tuple, &dflt_val, BPF_ANY) < 0)
> + return 0;
> + valp = bpf_map_lookup_elem(&tuples, tuple);
> + if (valp == 0)
> + return 0;
> + *valp = (uint64_t)valp;
> + if (bpf_map_update_elem(&tuples, tuple, valp, BPF_ANY) < 0)
> + return 0;
> +
> + val = *valp;
> + } else {
> + /*
> + * Record the value (used as key into the dvars map), and if we
> + * are storing a zero-value (deleting the element), delete the
> + * tuple. The associated dynamic variable will be delete by
will be delete
->
will be deleted
> + * the dt_get_dvar() call.
> + */
> + val = *valp;
> +
> + if (store && !nval)
> + bpf_map_delete_elem(&tuples, tuple);
> + }
> +
> + return dt_get_dvar(val, store, nval);
> +}
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> +static void
> +dt_cg_arglist(dt_ident_t *idp, dt_node_t *args, dt_irlist_t *dlp,
> + dt_regset_t *drp)
> +{
> + dtrace_hdl_t *dtp = yypcb->pcb_hdl;
> + const dt_idsig_t *isp = idp->di_data;
> + dt_node_t *dnp;
> + dt_ident_t *maxtupsz = dt_dlib_get_var(dtp, "TUPSZ");
> + int i;
> + int treg, areg;
> + uint_t tuplesize = sizeof(uint32_t);
> + size_t strsize = dtp->dt_options[DTRACEOPT_STRSIZE];
> +
> + TRACE_REGSET(" arglist: Begin");
> +
> + for (dnp = args, i = 0; dnp != NULL; dnp = dnp->dn_list, i++) {
> + dt_cg_node(dnp, dlp, drp);
> +
> + /* Push the component (pointer or value) onto the stack. */
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_SP));
> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_0, 0, dnp->dn_reg));
> + dt_regset_free(drp, dnp->dn_reg);
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, -DT_STK_SLOT_SZ));
> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DT_STK_SP, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
Need to check dnp->dn_reg == BPF_REG_0? I know "our register spill code
is brittle" but there is at least one other case (I think) of us being
careful about such accidents.
> + }
> +
> + if (i > dtp->dt_conf.dtc_diftupregs)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOTUPREG);
Why isn't this check inside that loop?
> + TRACE_REGSET(" arglist: Stack");
> +
> + if ((treg = dt_regset_alloc(drp)) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_LOAD(BPF_DW, treg, BPF_REG_FP, DT_STK_DCTX));
> + emit(dlp, BPF_LOAD(BPF_DW, treg, treg, DCTX_MEM));
> +
> + /*
> + * We need to clear the tuple assembly area in case the previous tuple
> + * was larger than the one we will construct, because otherwise we end
> + * up with trailing garbage.
> + */
Yeah. FWIW another approach is just to have an empty string (not an
entire empty tuple area). Then, if there is ever a string tuple arg,
just clear out the string before writing it rather than clearing out the
entire tuple area. That presumably reduces how much space is dedicated
to having only 0s. But not needed for this patch.
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_MOV_REG(BPF_REG_1, treg));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, DMEM_TUPLE(dtp)));
> + emite(dlp, BPF_MOV_IMM(BPF_REG_2, -1), maxtupsz);
> + emit(dlp, BPF_MOV_REG(BPF_REG_3, BPF_REG_1));
> + emite(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, -1), maxtupsz);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> + dt_regset_free_args(drp);
> + dt_regset_free(drp, BPF_REG_0);
> +
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, DMEM_TUPLE(dtp) + sizeof(uint32_t)));
> +
> + if ((areg = dt_regset_alloc(drp)) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + /* Deteermine first stack slot to read components from. */
Deteermine
> + emit(dlp, BPF_LOAD(BPF_DW, areg, BPF_REG_FP, DT_STK_SP));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, areg, i * DT_STK_SLOT_SZ));
> +
> +TRACE_REGSET(" arglist: DEBUG BEFORE LOOP");
> + for (dnp = args, i = 0; dnp != NULL; dnp = dnp->dn_list, i++) {
> + dtrace_diftype_t t;
> + size_t size;
> +
> + dt_node_diftype(dtp, dnp, &t);
> + size = t.dtdt_size;
> + if (size == 0)
> + continue;
> +
> + /* Append the value to the tuple assembly area. */
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, areg, -i * DT_STK_SLOT_SZ));
> +
> + dnp->dn_reg = isp->dis_args[i].dn_reg = BPF_REG_0;
> + dt_cg_typecast(dnp, &isp->dis_args[i], dlp, drp);
> + isp->dis_args[i].dn_reg = -1;
> +
> + if (dt_node_is_scalar(dnp) || dt_node_is_float(dnp)) {
> + assert(size > 0 && size <= 8 &&
> + (size & (size - 1)) == 0);
> +
> + emit(dlp, BPF_STORE(ldstw[size], treg, 0, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, size));
> +
> + tuplesize += size;
> + } else if (dt_node_is_string(dnp)) {
> + uint_t lbl_valid = dt_irlist_label(dlp);
> +
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_MOV_REG(BPF_REG_1, treg));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, strsize + 1));
> + emit(dlp, BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> + dt_cg_tstring_free(yypcb, dnp);
> + dt_regset_free(drp, BPF_REG_0);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read_str));
> + dt_regset_free_args(drp);
> + emit(dlp, BPF_BRANCH_IMM(BPF_JSGE, BPF_REG_0, 0, lbl_valid));
> + dt_regset_free(drp, BPF_REG_0);
> + dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 128 + i);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emitl(dlp, lbl_valid,
> + BPF_ALU64_REG(BPF_ADD, treg, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> +
> + tuplesize += size + 1;
> + } else if (t.dtdt_flags & DIF_TF_BYREF) {
> + uint_t lbl_valid = dt_irlist_label(dlp);
> +
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_MOV_REG(BPF_REG_1, treg));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> + emit(dlp, BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> + dt_regset_free_args(drp);
> + emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_valid));
> + dt_cg_probe_error(yypcb, DTRACEFLT_BADADDR, 0);
> + emitl(dlp, lbl_valid,
> + BPF_ALU64_IMM(BPF_ADD, treg, size));
> + dt_regset_free(drp, BPF_REG_0);
> +
> + tuplesize += size;
> + } else
> + assert(0); /* We shouldn't be able to get here. */
> + }
> +
> + /* Pop all components at once. */
> + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DT_STK_SP, areg));
> +
> + dt_regset_free(drp, areg);
> +
> + TRACE_REGSET(" arglist: Tuple");
> +
> + /* Add room for an optional TLS key (or 0). */
> + tuplesize += sizeof(uint64_t);
Should move the tuplesize update down to where tuplesize is actually used.
> + if (idp->di_flags & DT_IDFLG_TLS) {
> + dt_ident_t *idp = dt_dlib_get_func(dtp, "dt_tlskey");
> + uint_t varid = idp->di_id - DIF_VAR_OTHER_UBASE;
> +
> + assert(idp != NULL);
> +
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + emit(dlp, BPF_MOV_IMM(BPF_REG_1, varid));
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> + dt_regset_free_args(drp);
> + emit(dlp, BPF_STORE(BPF_DW, treg, 0, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> + } else
> + emit(dlp, BPF_STORE_IMM(BPF_DW, treg, 0, 0));
> +
> + if (tuplesize > dtp->dt_maxtuplesize)
> + dtp->dt_maxtuplesize = tuplesize;
> +
> + emit(dlp, BPF_LOAD(BPF_DW, treg, BPF_REG_FP, DT_STK_DCTX));
> + emit(dlp, BPF_LOAD(BPF_DW, treg, treg, DCTX_MEM));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, treg, DMEM_TUPLE(dtp)));
> +
> + args->dn_reg = treg;
> +
> + TRACE_REGSET(" arglist: End ");
> +}
> @@ -3176,75 +3381,59 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> static void
> dt_cg_assoc_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> {
> - ssize_t base;
> + dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_assoc");
> + uint_t varid;
> +
> + TRACE_REGSET(" assoc_op: Begin");
>
> + assert(idp != NULL);
> assert(dnp->dn_kind == DT_NODE_VAR);
> assert(!(dnp->dn_ident->di_flags & DT_IDFLG_LOCAL));
> assert(dnp->dn_args != NULL);
>
> + dnp->dn_ident->di_flags |= DT_IDFLG_DIFR;
> +
> dt_cg_arglist(dnp->dn_ident, dnp->dn_args, dlp, drp);
>
> if ((dnp->dn_reg = dt_regset_alloc(drp)) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> - if (dnp->dn_ident->di_flags & DT_IDFLG_TLS)
> - base = 0x2000;
> - else
> - base = 0x3000;
> + varid = dnp->dn_ident->di_id - DIF_VAR_OTHER_UBASE;
>
> - dnp->dn_ident->di_flags |= DT_IDFLG_DIFR;
> - emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, base + dnp->dn_ident->di_id));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_1, varid));
> + emit(dlp, BPF_MOV_REG(BPF_REG_2, dnp->dn_args->dn_reg));
> + dt_regset_free(drp, dnp->dn_args->dn_reg);
> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, 0));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_4, 0));
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> + dt_regset_free_args(drp);
>
> - /*
> - * If the associative array is a pass-by-reference type, then we are
> - * loading its value as a pointer to either load or store through it.
> - * The array element in question may not have been faulted in yet, in
> - * which case DIF_OP_LD*AA will return zero. We append an epilogue
> - * of instructions similar to the following:
> - *
> - * ld?aa id, %r1 ! base ld?aa instruction above
> - * tst %r1 ! start of epilogue
> - * +--- bne label
> - * | setx size, %r1
> - * | allocs %r1, %r1
> - * | st?aa id, %r1
> - * | ld?aa id, %r1
> - * v
> - * label: < rest of code >
> - *
> - * The idea is that we allocs a zero-filled chunk of scratch space and
> - * do a DIF_OP_ST*AA to fault in and initialize the array element, and
> - * then reload it to get the faulted-in address of the new variable
> - * storage. This isn't cheap, but pass-by-ref associative array values
> - * are (thus far) uncommon and the allocs cost only occurs once. If
> - * this path becomes important to DTrace users, we can improve things
> - * by adding a new DIF opcode to fault in associative array elements.
> - */
> if (dnp->dn_flags & DT_NF_REF) {
> -#ifdef FIXME
> - uint_t stvop = op == DIF_OP_LDTAA ? DIF_OP_STTAA : DIF_OP_STGAA;
> - uint_t label = dt_irlist_label(dlp);
> -
> - emit(dlp, BPF_BRANCH_IMM(BPF_JNE, dnp->dn_reg, 0, label));
> -
> - dt_cg_setx(dlp, dnp->dn_reg, dt_node_type_size(dnp));
> - instr = DIF_INSTR_ALLOCS(dnp->dn_reg, dnp->dn_reg);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> -
> - dnp->dn_ident->di_flags |= DT_IDFLG_DIFW;
> - instr = DIF_INSTR_STV(stvop, dnp->dn_ident->di_id, dnp->dn_reg);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> -
> - instr = DIF_INSTR_LDV(op, dnp->dn_ident->di_id, dnp->dn_reg);
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + emit(dlp, BPF_MOV_REG(dnp->dn_reg, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> + } else {
> + size_t size = dt_node_type_size(dnp);
> + 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);
Why assert() spread over two lines? Or just define a macro for all
these assert(size...) instances and use it.
> + 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));
> + dt_regset_free(drp, BPF_REG_0);
>
> - emitl(dlp, label,
> + emitl(dlp, lbl_done,
> BPF_NOP());
> -#else
> - xyerror(D_UNKNOWN, "internal error -- no support for "
> - "associative arrays yet\n");
> -#endif
> }
> +
> + TRACE_REGSET(" assoc_op: End ");
> }
>
> static void
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index d45720d7..dc2f8e58 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
Incidentally the copyright message for this file looks weird.
> @@ -72,9 +72,13 @@ typedef struct dt_dctx {
> * +----------------+----------------+
> * 0 -> | Stack : tstring | \
> * | trace (shared) storage | |
> - * | storage : | > DMEM_SIZE
> + * | storage : | |
> * +----------------+----------------+ |
> - * DMEM_STRTOK -> | strtok() internal state | /
> + * DMEM_STRTOK -> | strtok() internal state | > DMEM_SIZE
> + * +---------------------------------+ |
> + * DMEM_TUPLE -> | tuple assembly area | |
> + * +---------------------------------+ |
> + * DMEM_TUPLE_DFLT -> | default empty tuple | /
> * +---------------------------------+
> */
>
> @@ -88,17 +92,23 @@ typedef struct dt_dctx {
> P2ROUNDUP((dtp)->dt_options[DTRACEOPT_STRSIZE] + 1, 8))
> #define DMEM_STRTOK_SZ(dtp) \
> (sizeof(uint64_t) + (dtp)->dt_options[DTRACEOPT_STRSIZE] + 1)
> +#define DMEM_TUPLE_SZ(dtp) \
> + ((dtp)->dt_maxtuplesize)
>
> /*
> - * Macro to determine the offset from mem to the strtok internal state.
> + * Macros to determine the offset of the components of dctx->mem.
> */
> #define DMEM_STRTOK(dtp) \
> MAX(DMEM_STACK_SZ(dtp), DMEM_TSTR_SZ(dtp))
> +#define DMEM_TUPLE(dtp) \
> + (DMEM_STRTOK(dtp) + DMEM_STRTOK_SZ(dtp))
> +#define DMEM_TUPLE_DFLT(dtp) \
> + (DMEM_TUPLE(dtp) + DMEM_TUPLE_SZ(dtp))
>
> /*
> * Macro to determine the total size of the mem area.
> */
> -#define DMEM_SIZE(dtp) (DMEM_STRTOK(dtp) + DMEM_STRTOK_SZ(dtp))
> +#define DMEM_SIZE(dtp) (DMEM_TUPLE_DFLT(dtp) + DMEM_TUPLE_SZ(dtp))
>
> /*
> * The stack layout for BPF programs that are generated as trampolines for
I sent out earlier comments that defining
DMEM_TUPLE_SZ(dtp) ((dtp)->dt_maxtuplesize)
DMEM_TUPLE_DFLT(dtp) (DMEM_TUPLE(dtp) + DMEM_TUPLE_SZ(dtp))
DMEM_SIZE(dtp) (DMEM_TUPLE_DFLT(dtp) + DMEM_TUPLE_SZ(dtp))
strikes me as dangerous since they depend on dtp->dt_maxtuplesize and
therefore change value in a way that other similar defines do not. I
recommend in the schematic, change
+---------------------------------+
DMEM_TUPLE -> | tuple assembly area |
+---------------------------------+
DMEM_TUPLE_DFLT -> | default empty tuple |
+---------------------------------+
to
+---------------------------------+
DMEM_TUPLE -> | tuple assembly area |
| + |
| default empty tuple |
+---------------------------------+
to get rid of DMEM_TUPLE_DFLT. It's only used here and its presence
raises the question why it isn't being used in, say, dt_cg_arglist().
Get rid of the DMEM_TUPLE_DFLT definition.
Just make DMEM_SIZE is just DMEM_TUPLE(dtp) + 2 * tuple size.
Remove DMEM_TUPLE_SZ and just use ((dtp)->dt_maxtuplesize).
> diff --git a/test/unittest/assocs/tst.gvar-postdec.d b/test/unittest/assocs/tst.gvar-postdec.d
> new file mode 100644
> index 00000000..17ed5edb
> --- /dev/null
> +++ b/test/unittest/assocs/tst.gvar-postdec.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Post-decrement should work for global associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = a["a"] = 42;
> + val = a["a"]--;
> +
> + exit(val != old || a["a"] != old - 1);
> +}
> diff --git a/test/unittest/assocs/tst.gvar-postinc.d b/test/unittest/assocs/tst.gvar-postinc.d
> new file mode 100644
> index 00000000..69fb1047
> --- /dev/null
> +++ b/test/unittest/assocs/tst.gvar-postinc.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Post-increment should work for global associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = a["a"] = 42;
> + val = a["a"]++;
> +
> + exit(val != old || a["a"] != old + 1);
> +}
> diff --git a/test/unittest/assocs/tst.gvar-predec.d b/test/unittest/assocs/tst.gvar-predec.d
> new file mode 100644
> index 00000000..36b6917d
> --- /dev/null
> +++ b/test/unittest/assocs/tst.gvar-predec.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Pre-decrement should work for global associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = a["a"] = 42;
> + val = --a["a"];
> +
> + exit(val != old - 1);
> +}
> diff --git a/test/unittest/assocs/tst.gvar-preinc.d b/test/unittest/assocs/tst.gvar-preinc.d
> new file mode 100644
> index 00000000..ad0d8b9d
> --- /dev/null
> +++ b/test/unittest/assocs/tst.gvar-preinc.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Pre-increment should work for global associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = a["a"] = 42;
> + val = ++a["a"];
> +
> + exit(val != old + 1);
> +}
In
test/unittest/assocs/tst.gvar-predec.d
test/unittest/assocs/tst.gvar-preinc.d
check the value of a["a"] just as you do in
test/unittest/assocs/tst.gvar-postdec.d
test/unittest/assocs/tst.gvar-postinc.d
Same issue in the tvar variants of these tests.
> diff --git a/test/unittest/assocs/tst.init-str.d b/test/unittest/assocs/tst.init-str.d
> new file mode 100644
> index 00000000..2894fd68
> --- /dev/null
> +++ b/test/unittest/assocs/tst.init-str.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Default value of unassigned elements in asssociative string
> + * arrays is 0. This will cause an 'invalid address 0x0' fault
> + * upon dereferencing.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +string s[int];
> +
> +BEGIN
> +{
> + trace(s[1]);
> + exit(1);
> +}
> +
> +ERROR
> +{
> + exit(arg4 != 1 || arg5 != 0);
> +}
> diff --git a/test/unittest/assocs/tst.init.d b/test/unittest/assocs/tst.init.d
> new file mode 100644
> index 00000000..567898b6
> --- /dev/null
> +++ b/test/unittest/assocs/tst.init.d
> @@ -0,0 +1,21 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Default value of unassigned elements in asssociative arrays is 0
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + a["a"] = 42;
> +
> + exit(a["b"] != 0);
> +}
> diff --git a/test/unittest/assocs/tst.misc.d b/test/unittest/assocs/tst.misc.d
> index 51474ae4..ee0f33df 100644
> --- a/test/unittest/assocs/tst.misc.d
> +++ b/test/unittest/assocs/tst.misc.d
> @@ -1,10 +1,9 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION:
> diff --git a/test/unittest/assocs/tst.nested.d b/test/unittest/assocs/tst.nested.d
> new file mode 100644
> index 00000000..0e491460
> --- /dev/null
> +++ b/test/unittest/assocs/tst.nested.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Nested tuple assembly works correctly.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + a[1, 2, 3] = 1234;
> + a[3, 2, 3] = 4321;
> + a[3, 2, 1] = 2;
> + printf("%d\n", a[1, a[3, 2, 1], 3]);
> +
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/assocs/tst.nested.r b/test/unittest/assocs/tst.nested.r
> new file mode 100644
> index 00000000..2286d4f0
> --- /dev/null
> +++ b/test/unittest/assocs/tst.nested.r
> @@ -0,0 +1,2 @@
> +1234
> +
> diff --git a/test/unittest/assocs/tst.nested2.d b/test/unittest/assocs/tst.nested2.d
> new file mode 100644
> index 00000000..803df84c
> --- /dev/null
> +++ b/test/unittest/assocs/tst.nested2.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Nested tuple assembly works correctly.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + a[1, 2, 3] = 1234;
> + a[3, 2, 3] = 4321;
> + a[3, 2, 1] = 2;
> + printf("%d\n", a[1, a[3, a[3, a[3, a[3, 2, 1], 1], 1], 1], 3]);
> +
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/assocs/tst.nested2.r b/test/unittest/assocs/tst.nested2.r
> new file mode 100644
> index 00000000..2286d4f0
> --- /dev/null
> +++ b/test/unittest/assocs/tst.nested2.r
> @@ -0,0 +1,2 @@
> +1234
> +
> diff --git a/test/unittest/assocs/tst.orthogonality.d b/test/unittest/assocs/tst.orthogonality.d
> index 0ae50d08..b87211ea 100644
> --- a/test/unittest/assocs/tst.orthogonality.d
> +++ b/test/unittest/assocs/tst.orthogonality.d
> @@ -1,10 +1,9 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * This test confirms the orthogonality of associative arrays and thread-local
> diff --git a/test/unittest/assocs/tst.store_zero_deletes.d b/test/unittest/assocs/tst.store_zero_deletes.d
> new file mode 100644
> index 00000000..2db9c6c0
> --- /dev/null
> +++ b/test/unittest/assocs/tst.store_zero_deletes.d
> @@ -0,0 +1,35 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Storing 0 into an associate array element removes it from
> + * storage, making room for another element.
> + *
> + * SECTION: Variables/Thread-Local Variables
> + */
> +
> +#pragma D option quiet
> +#pragma D option dynvarsize=15
> +
> +BEGIN
> +{
> + a[1] = 1;
> + a[2] = 2;
> + a[3] = 3;
> + a[1] = 0;
> + a[4] = 4;
> + trace(a[1]);
> + trace(a[2]);
> + trace(a[3]);
> + trace(a[4]);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/assocs/tst.store_zero_deletes.r b/test/unittest/assocs/tst.store_zero_deletes.r
> new file mode 100644
> index 00000000..0f6f3a7d
> --- /dev/null
> +++ b/test/unittest/assocs/tst.store_zero_deletes.r
> @@ -0,0 +1 @@
> +0234
> diff --git a/test/unittest/assocs/tst.tvar-postdec.d b/test/unittest/assocs/tst.tvar-postdec.d
> new file mode 100644
> index 00000000..a8f6bf7f
> --- /dev/null
> +++ b/test/unittest/assocs/tst.tvar-postdec.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Post-decrement should work for TLS associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = self->a["a"] = 42;
> + val = self->a["a"]--;
> +
> + exit(val != old || self->a["a"] != old - 1);
> +}
> diff --git a/test/unittest/assocs/tst.tvar-postinc.d b/test/unittest/assocs/tst.tvar-postinc.d
> new file mode 100644
> index 00000000..e9a50763
> --- /dev/null
> +++ b/test/unittest/assocs/tst.tvar-postinc.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Post-increment should work for TLS associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = self->a["a"] = 42;
> + val = self->a["a"]++;
> +
> + exit(val != old || self->a["a"] != old + 1);
> +}
> diff --git a/test/unittest/assocs/tst.tvar-predec.d b/test/unittest/assocs/tst.tvar-predec.d
> new file mode 100644
> index 00000000..8b85b990
> --- /dev/null
> +++ b/test/unittest/assocs/tst.tvar-predec.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Pre-decrement should work for TLS associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = self->a["a"] = 42;
> + val = --self->a["a"];
> +
> + exit(val != old - 1);
> +}
> diff --git a/test/unittest/assocs/tst.tvar-preinc.d b/test/unittest/assocs/tst.tvar-preinc.d
> new file mode 100644
> index 00000000..ddea5609
> --- /dev/null
> +++ b/test/unittest/assocs/tst.tvar-preinc.d
> @@ -0,0 +1,22 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Pre-increment should work for TLS associative arrays.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + old = self->a["a"] = 42;
> + val = ++self->a["a"];
> +
> + exit(val != old + 1);
> +}
More information about the DTrace-devel
mailing list