[DTrace-devel] [PATCH 3/6] Change storage for local variables
Kris Van Hees
kris.van.hees at oracle.com
Wed Mar 31 14:06:15 PDT 2021
Note that for this and global vars, you are limited to the max kmalloc
size for a map value. Since this is per-CPU you have even less room because
the NR_CPUS * var-data must be less than the max kmalloc size.
This is why I suggested looked into the implementation using multiple map
entries, akin to memory pages. It complicates memory access a bit of course
but it avoids the limitation. For gvars the limit isn't as bad (but still an
issue once you start using strings or structs) but for local variables you can
hit the limit auite quickly.
That will need to be a (near) future enhancement though.
Review comments below...
On Fri, Mar 19, 2021 at 01:45:31PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> Change how local variables are stored and accessed. Instead of
> storing them as 8-byte quantities on the stack addressed by variable
> ID, just use a monolithic, per-CPU BPF map addressed by a variable's
"use the value of a single-key BPF map addressed"
> offset. This allows us to store local variables of "arbitrary" size
> (e.g., > 8 bytes) and worry less about running out of stack space.
> A pointer to the BPF map is kept on the stack in the DTrace context.
"to the BPF map value"
> In general, local-variable access now greatly mimics global-variable
> access.
>
> A dtp field dt_maxlvaralloc tracks the largest local-variable
> allocation that will be needed for any pcb.
>
> This patch also backs out patch
> "a4d5f5bda15d Ensure that local variables get a 0 value upon first load",
> which changed dt_cg_load_var() to load a 0 when a clause-local variable
> is read but not yet initialized in that clause. Note that:
> x Such behavior is not needed for D language compliance -- see:
> https://docs.oracle.com/en/operating-systems/oracle-linux
> /dtrace-guide/dt_dlang.html#dt_clocal_dlang
> which states, "Each clause-local variable is instantiated with an
> undefined value the first time it is used in the script."
Sure, although init to 0 is perfectly valid. I think it is sufficient to
just mention it is not required for language compliance. No need to out in
a link etc, That makes it seem like the previous behaviour was wrong which
is not the case,
> x The old patch was motivated by a BPF verifier issue when local
> variables were kept on the stack, which is no longer the case.
Correct.
> x The old patch implemented this behavior for each clause, even though
> clause-local variables should be persistent from clause to clause
> (within the same probe firing). Thus, the old patch was faulty and
> causing test/demo/vars/clause.d to fail.
Correct, that was a bug.
You should mention that this patch breaks the annotations in the disassembler.
You do fix that in a later patch, but it is introducing a breakage (flagged as
FIXME in the code).
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> BPF-NOTES | 12 +-
> libdtrace/dt_bpf.c | 12 +-
> libdtrace/dt_cc.c | 6 +-
> libdtrace/dt_cg.c | 56 +++---
> libdtrace/dt_dctx.h | 76 ++------
> libdtrace/dt_dis.c | 2 +-
> libdtrace/dt_dlibs.c | 1 +
> libdtrace/dt_impl.h | 1 +
> libdtrace/dt_pcb.c | 3 +
> test/demo/vars/clause.d | 3 +-
> test/unittest/codegen/tst.stack_layout.r | 28 ++-
> test/unittest/struct/tst.global_clauselocal.d | 163 ++++++++++++++++++
> test/unittest/struct/tst.global_clauselocal.r | 7 +
> .../variables/lvar/tst.load_before_store.d | 9 +-
> test/unittest/variables/lvar/tst.struct.d | 5 +-
> test/utils/print-stack-layout.c | 18 +-
> 16 files changed, 260 insertions(+), 142 deletions(-)
> create mode 100644 test/unittest/struct/tst.global_clauselocal.d
> create mode 100644 test/unittest/struct/tst.global_clauselocal.r
>
> diff --git a/BPF-NOTES b/BPF-NOTES
> index 27c2900e..20e2f204 100644
> --- a/BPF-NOTES
> +++ b/BPF-NOTES
> @@ -32,15 +32,9 @@
> add %rX, IMM stb [%rX+IMM], %rY
> stb [%rX+0], %rY
>
> -- Local variables can be stored on the stack, although that will limit us to
> - 64 local variables (or 32, if we reserve half the stack as scratch space for
> - constructing strings and map keys). That ought to be enough. Note that
> - variables that store strings or structures that are larger than 8 bytes are
> - stored by reference, i.e. the slot on the stack stores an offset into a
> - memory block obtained as map value from an array map.
> -
> - This means that in its most basic form, local variable i can be loaded from
> - [%fp-(i*8)-8].
> +- Local variables can be stored in a per-CPU BPF map, which can be a large,
> + single block of memory. Access to local variables can be similar to how
> + we handle global variables.
You may want to move this below the global variables dwscription since you are
reference that one. Back ref is usually more clear than forward ref.
>
> - Global variables can be stored in a BPF map, which can be a large, single
> block of memory. Variables can be accessed with three BPF instructions:
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index e33f6afd..f67764f3 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -173,6 +173,8 @@ set_task_offsets(dtrace_hdl_t *dtp)
> * consumer handle (dt_strlen).
> * - gvars: Global variables map. This is a global map with a singleton
> * element (key 0) addressed by variable offset.
> + * - lvars: Local variables map. This is a per-CPU map with a singleton
> + * element (key 0) addressed by variable offset.
> *
> * FIXME: TLS variable storage is still being designed further so this is just
> * a temporary placeholder and will most likely be replaced by something
> @@ -195,7 +197,7 @@ set_task_offsets(dtrace_hdl_t *dtp)
> int
> dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> {
> - int gvarsz, tvarc, aggsz;
> + int gvarsz, lvarsz, tvarc, aggsz;
> int ci_mapfd;
> uint32_t key = 0;
>
> @@ -209,8 +211,9 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> /* Determine the aggregation buffer size. */
> aggsz = dt_idhash_datasize(dtp->dt_aggs);
>
> - /* Determine the number of global and TLS variables. */
> + /* Determine sizes for global, local, and TLS maps. */
> gvarsz = (dt_idhash_datasize(dtp->dt_globals) + 7) & ~7;
> + lvarsz = (dtp->dt_maxlvaralloc + 7) & ~7;
Use roundup or P2ROUNDUP.
> tvarc = dt_idhash_peekid(dtp->dt_tls) - DIF_VAR_OTHER_UBASE;
>
> /* Create global maps as long as there are no errors. */
> @@ -257,6 +260,11 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> sizeof(uint32_t), gvarsz, 1) == -1)
> return -1; /* dt_errno is set for us */
>
> + if (lvarsz > 0 &&
> + create_gmap(dtp, "lvars", BPF_MAP_TYPE_PERCPU_ARRAY,
> + sizeof(uint32_t), lvarsz, 1) == -1)
> + return -1; /* dt_errno is set for us */
> +
> if (tvarc > 0 &&
> create_gmap(dtp, "tvars", BPF_MAP_TYPE_ARRAY,
> sizeof(uint32_t), sizeof(uint64_t), tvarc) == -1)
> diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> index b7362e4b..2fe91ae4 100644
> --- a/libdtrace/dt_cc.c
> +++ b/libdtrace/dt_cc.c
> @@ -2082,8 +2082,8 @@ dt_compile(dtrace_hdl_t *dtp, int context, dtrace_probespec_t pspec, void *arg,
> longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
>
> yypcb->pcb_idents = dt_idhash_create("ambiguous", NULL, 0, 0);
> - yypcb->pcb_locals = dt_idhash_create("clause local", NULL,
> - 0, DT_LVAR_MAX);
> + yypcb->pcb_locals = dt_idhash_create("clause local", NULL, 0,
> + UINT_MAX);
UINT_MAX); fits on the line above
> if (yypcb->pcb_idents == NULL || yypcb->pcb_locals == NULL)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> @@ -2244,7 +2244,7 @@ dt_construct(dtrace_hdl_t *dtp, dt_probe_t *prp, uint_t cflags, dt_ident_t *idp)
>
> yypcb->pcb_idents = dt_idhash_create("ambiguous", NULL, 0, 0);
> yypcb->pcb_locals = dt_idhash_create("clause local", NULL, 0,
> - DT_LVAR_MAX);
> + UINT_MAX);
UINT_MAX); fits on the line above
> if (yypcb->pcb_idents == NULL || yypcb->pcb_locals == NULL)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index ab3fe761..da3e9ca6 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -165,6 +165,8 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> DT_CG_STORE_MAP_PTR("aggs", DCTX_AGG);
> if (dt_idhash_datasize(dtp->dt_globals) > 0)
> DT_CG_STORE_MAP_PTR("gvars", DCTX_GVARS);
> + if (dtp->dt_maxlvaralloc > 0)
> + DT_CG_STORE_MAP_PTR("lvars", DCTX_LVARS);
> #undef DT_CG_STORE_MAP_PTR
> }
>
> @@ -1613,30 +1615,24 @@ 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);
> + int map_ptr_offset = -1;
It is not the offset of a BPF map pointer in the dctx, but rather the offset
at which you stored the map value that is used to store the variable data. So,
perhaps a better name would simply be dctx_offset? Or just forget about using
a variable for this. Eventually TLS will use the same mechanism anyway.
> idp->di_flags |= DT_IDFLG_DIFR;
> - if (idp->di_flags & DT_IDFLG_LOCAL) { /* local var */
> - if ((dst->dn_reg = dt_regset_alloc(drp)) == -1)
> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> - /*
> - * If this is the first read for this local variable, we know
> - * the value is 0. This avoids storing an initial 0 value in
> - * the variable's stack location.
> - */
> - if (!(idp->di_flags & DT_IDFLG_DIFW))
> - emit(dlp, BPF_MOV_IMM(dst->dn_reg, 0));
> - else
> - emit(dlp, BPF_LOAD(BPF_DW, dst->dn_reg, BPF_REG_FP, DT_STK_LVAR(idp->di_id)));
> - return;
> - }
> -
> - /* FIXME: need cleaner test for global var */
> - if (!(idp->di_flags & DT_IDFLG_TLS) && idp->di_id >= DIF_VAR_OTHER_UBASE) {
> + /* first handle local and global variables */
> + if (idp->di_flags & DT_IDFLG_LOCAL)
> + map_ptr_offset = DCTX_LVARS;
> + else if (!(idp->di_flags & DT_IDFLG_TLS) && idp->di_id >= DIF_VAR_OTHER_UBASE)
> + map_ptr_offset = DCTX_GVARS;
Since I really think map_ptr_offset is both the wrong name and not needed, just
have a code section that handles lvar and gvar, and then the rest is TLS (for
now). In that lvar/gvar case, just generate the appropriate instruction to
access the map value for the appropriate variable kind. That is what you do
in dt_cg_store_var() anyway.
> + if (map_ptr_offset != -1) {
> if ((dst->dn_reg = dt_regset_alloc(drp)) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> + /* get the BPF map pointer */
> emit(dlp, BPF_LOAD(BPF_DW, dst->dn_reg, BPF_REG_FP, DT_STK_DCTX));
> - emit(dlp, BPF_LOAD(BPF_DW, dst->dn_reg, dst->dn_reg, DCTX_GVARS));
> + emit(dlp, BPF_LOAD(BPF_DW, dst->dn_reg, dst->dn_reg, map_ptr_offset));
> +
> + /* load the variable value or address */
> if ((dst->dn_flags & DT_NF_REF) == 0)
> emit(dlp, BPF_LOAD(BPF_DW, dst->dn_reg, dst->dn_reg, idp->di_offset));
> else
> @@ -1645,6 +1641,7 @@ dt_cg_load_var(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
> return;
> }
>
> + /* otherwise, handle thread-local and built-in variables */
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> @@ -1913,32 +1910,35 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
> uint_t varid;
>
> idp->di_flags |= DT_IDFLG_DIFW;
> - if (idp->di_flags & DT_IDFLG_LOCAL) { /* local var */
> - emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DT_STK_LVAR(idp->di_id), src->dn_reg));
> - return;
> - }
> -
> - varid = idp->di_id - DIF_VAR_OTHER_UBASE;
>
> - if (idp->di_flags & DT_IDFLG_TLS) /* TLS var */
> - idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_set_tvar");
> - else { /* global var */
> + /* global and local variables (that is, not thread-local) */
> + if (!(idp->di_flags & DT_IDFLG_TLS)) {
> int reg;
>
> + /* get pointer to BPF map (either lvars or gvars) */
> if ((reg = dt_regset_alloc(drp)) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> emit(dlp, BPF_LOAD(BPF_DW, reg, BPF_REG_FP, DT_STK_DCTX));
> - emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DCTX_GVARS));
> + if (idp->di_flags & DT_IDFLG_LOCAL)
> + emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DCTX_LVARS));
> + else
> + emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DCTX_GVARS));
> +
> + /* store by value or by reference */
> if ((src->dn_flags & DT_NF_REF) == 0)
> emit(dlp, BPF_STORE(BPF_DW, reg, idp->di_offset, src->dn_reg));
> else {
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
> dt_cg_memcpy(dlp, drp, reg, src->dn_reg, idp->di_size);
> }
> +
> dt_regset_free(drp, reg);
> return;
> }
>
> + /* TLS var */
> + varid = idp->di_id - DIF_VAR_OTHER_UBASE;
> + idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_set_tvar");
> assert(idp != NULL);
>
> if (dt_regset_xalloc_args(drp) == -1)
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index 2707f9fd..0873602c 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
> @@ -40,6 +40,7 @@ typedef struct dt_dctx {
> char *buf; /* Output buffer scratch memory */
> char *agg; /* Aggregation data */
> char *gvars; /* Global variables */
> + char *lvars; /* Local variables */
> } dt_dctx_t;
>
> #define DCTX_CTX offsetof(dt_dctx_t, ctx)
> @@ -48,6 +49,7 @@ typedef struct dt_dctx {
> #define DCTX_BUF offsetof(dt_dctx_t, buf)
> #define DCTX_AGG offsetof(dt_dctx_t, agg)
> #define DCTX_GVARS offsetof(dt_dctx_t, gvars)
> +#define DCTX_LVARS offsetof(dt_dctx_t, lvars)
> #define DCTX_SIZE ((int16_t)sizeof(dt_dctx_t))
>
> /*
> @@ -72,50 +74,27 @@ typedef struct dt_dctx {
> */
> #define DT_STK_NREGS (MAX_BPF_REG - 2)
>
> -/*
> - * An area of 256 bytes is set aside on the stack as scratch area for things
> - * like string operations. It extends from the end of the stack towards the
> - * local variable storage area. DT_STK_LVAR_END is therefore the last byte
> - * location on the stack that can be used for local variable storage.
> - */
> -#define DT_STK_SCRATCH_BASE (-MAX_BPF_STACK)
> -#define DT_STK_SCRATCH_SZ (256)
> -#define DT_STK_LVAR_END (DT_STK_SCRATCH_BASE + DT_STK_SCRATCH_SZ)
> -
> /*
> * The stack layout for functions that implement a D clause is encoded with the
> * following constants.
> *
> * Note: The BPF frame pointer points to the address of the first byte past the
> - * end of the stack. If the stack size is 512 bytes, valid offsets are
> - * -1 through -512 (inclusive), So, the first 64-bit value on the stack
> - * occupies bytes at offsets -8 through -1. The second -16 through -9,
> - * and so on... 64-bit values are properly aligned at offsets -n where
> - * n is a multiple of 8 (sizeof(uint64_t)).
> - *
> - * The following diagram shows the stack layout for a size of 512 bytes.
> + * end of the stack. 8-byte values are properly aligned at offsets -8,
> + * -16, -24, -32, etc. -- that is, negative multiples of sizeof(uint64_t).
> *
> - * +----------------+
> - * SCRATCH_BASE = -512 | Scratch Memory |
> - * +----------------+
> - * LVAR_END = LVAR(n) = -256 | LVAR n | (n = DT_LVAR_MAX = 17)
> - * +----------------+
> - * | ... |
> - * +----------------+
> - * LVAR(1) = -128 | LVAR 1 |
> - * +----------------+
> - * LVAR_BASE = LVAR(0) = -120 | LVAR 0 |
> - * +----------------+
> - * SPILL(n) = -112 | %r8 | (n = DT_STK_NREGS - 1 = 8)
> - * +----------------+
> - * | ... |
> - * +----------------+
> - * SPILL(1) = -56 | %r1 |
> - * +----------------+
> - * SPILL_BASE = SPILL(0) = -48 | %r0 |
> - * +----------------+
> - * DCTX = -40 | DTrace Context | -1
> - * +----------------+
> + * +----------------+
> + * SCRATCH_BASE | Scratch Memory |
> + * +----------------+
> + * SPILL(n) | %r8 | (n = DT_STK_NREGS - 1 = 8)
> + * +----------------+
> + * | ... |
> + * +----------------+
> + * SPILL(1) | %r1 |
> + * +----------------+
> + * SPILL_BASE = SPILL(0) | %r0 |
> + * +----------------+
> + * DCTX | DTrace Context |
> + * +----------------+
> */
> #define DT_STK_BASE ((int16_t)0)
> #define DT_STK_SLOT_SZ ((int16_t)sizeof(uint64_t))
> @@ -123,25 +102,8 @@ typedef struct dt_dctx {
> #define DT_STK_DCTX (DT_STK_BASE - DCTX_SIZE)
> #define DT_STK_SPILL_BASE (DT_STK_DCTX - DT_STK_SLOT_SZ)
> #define DT_STK_SPILL(n) (DT_STK_SPILL_BASE - (n) * DT_STK_SLOT_SZ)
> -#define DT_STK_LVAR_BASE (DT_STK_SPILL(DT_STK_NREGS - 1) - \
> - DT_STK_SLOT_SZ)
> -#define DT_STK_LVAR(n) (DT_STK_LVAR_BASE - (n) * DT_STK_SLOT_SZ)
>
> -/*
> - * Calculate a local variable ID based on a given stack offset. If the stack
> - * offset is outside the valid range, this should evaluate as -1.
> - */
> -#define DT_LVAR_OFF2ID(n) (((n) > DT_STK_LVAR_BASE || \
> - (n) < DT_STK_LVAR_END) ? -1 : \
> - (-((n) - DT_STK_LVAR_BASE) / DT_STK_SLOT_SZ))
> -
> -/*
> - * Maximum number of local variables stored by value (scalars). This is bound
> - * by the choice to store them on the stack between the register spill space,
> - * and 256 bytes set aside as string scratch space. We also use the fact that
> - * the (current) maximum stack space for BPF programs is 512 bytes.
> - */
> -#define DT_LVAR_MAX (-(DT_STK_LVAR_END - DT_STK_LVAR_BASE) / \
> - DT_STK_SLOT_SZ)
> +#define DT_STK_SCRATCH_BASE (-MAX_BPF_STACK)
> +#define DT_STK_SCRATCH_SZ (MAX_BPF_STACK + DT_STK_SPILL(DT_STK_NREGS - 1))
>
> #endif /* _DT_DCTX_H */
> diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> index d5ea79ca..d4f4ad67 100644
> --- a/libdtrace/dt_dis.c
> +++ b/libdtrace/dt_dis.c
> @@ -68,7 +68,7 @@ dt_dis_lvarname(const dtrace_difo_t *dp, int reg, int var, uint_t addr,
> char *buf, int len)
> {
> if (reg == BPF_REG_FP) {
> - var = DT_LVAR_OFF2ID(var);
> + var = -1; /* FIXME: need to find variable offset */
This is why I mentioned above that this needs to be mentioned in the commit
msg.
> if (var != -1) {
> const char *vname;
>
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index 2c293719..96f7c49a 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -66,6 +66,7 @@ static const dt_ident_t dt_bpf_symbols[] = {
> DT_BPF_SYMBOL(buffers, DT_IDENT_PTR),
> DT_BPF_SYMBOL(cpuinfo, DT_IDENT_PTR),
> DT_BPF_SYMBOL(gvars, DT_IDENT_PTR),
> + DT_BPF_SYMBOL(lvars, DT_IDENT_PTR),
> DT_BPF_SYMBOL(mem, DT_IDENT_PTR),
> DT_BPF_SYMBOL(state, DT_IDENT_PTR),
> DT_BPF_SYMBOL(strtab, DT_IDENT_PTR),
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index 5dee86e6..d782362f 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -252,6 +252,7 @@ struct dtrace_hdl {
> char *dt_strtab; /* global string table (runtime) */
> uint_t dt_strlen; /* global string table (runtime) size */
> uint_t dt_maxreclen; /* largest record size across programs */
> + uint_t dt_maxlvaralloc; /* largest lvar alloc across pcbs */
> dt_list_t dt_modlist; /* linked list of dt_module_t's */
> dt_module_t **dt_mods; /* hash table of dt_module_t's */
> uint_t dt_modbuckets; /* number of module hash buckets */
> diff --git a/libdtrace/dt_pcb.c b/libdtrace/dt_pcb.c
> index d2e8c3a7..b10c688e 100644
> --- a/libdtrace/dt_pcb.c
> +++ b/libdtrace/dt_pcb.c
> @@ -158,6 +158,9 @@ dt_pcb_pop(dtrace_hdl_t *dtp, int err)
> ctf_discard(dtp->dt_ddefs->dm_ctfp);
> }
>
> + if (dtp->dt_maxlvaralloc < pcb->pcb_locals->dh_nextoff)
> + dtp->dt_maxlvaralloc = pcb->pcb_locals->dh_nextoff;
> +
> if (pcb->pcb_pragmas != NULL)
> dt_idhash_destroy(pcb->pcb_pragmas);
> if (pcb->pcb_locals != NULL)
> diff --git a/test/demo/vars/clause.d b/test/demo/vars/clause.d
> index bc332c50..7daa76f9 100644
> --- a/test/demo/vars/clause.d
> +++ b/test/demo/vars/clause.d
> @@ -1,11 +1,10 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2005, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2005, 2021, 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 */
> /* @@trigger: none */
>
> int me; /* an integer global variable */
> diff --git a/test/unittest/codegen/tst.stack_layout.r b/test/unittest/codegen/tst.stack_layout.r
> index 78d41c82..3a23b689 100644
> --- a/test/unittest/codegen/tst.stack_layout.r
> +++ b/test/unittest/codegen/tst.stack_layout.r
> @@ -1,17 +1,13 @@
> Base: 0
> -dctx: -40
> -%r0: -48
> -%r1: -56
> -%r2: -64
> -%r3: -72
> -%r4: -80
> -%r5: -88
> -%r6: -96
> -%r7: -104
> -%r8: -112
> -lvar[ -1]: -119 (ID -1)
> -lvar[ 0]: -120 (ID 0)
> -lvar[ 1]: -128 (ID 1)
> -lvar[ 17]: -256 (ID 17)
> -lvar[ -1]: -257 (ID -1)
> -scratch: -257 .. -512
> +dctx: -56
> +%r0: -64
> +%r1: -72
> +%r2: -80
> +%r3: -88
> +%r4: -96
> +%r5: -104
> +%r6: -112
> +%r7: -120
> +%r8: -128
> +DT_STK_SCRATCH_BASE: -512
> +DT_STK_SCRATCH_SZ: 384
Please stick with the format I had so that this will be:
scratch: -129 .. -512
> diff --git a/test/unittest/struct/tst.global_clauselocal.d b/test/unittest/struct/tst.global_clauselocal.d
> new file mode 100644
> index 00000000..6d45bc24
> --- /dev/null
> +++ b/test/unittest/struct/tst.global_clauselocal.d
> @@ -0,0 +1,163 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2021, 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: Verify assignments between global and clause-local variables.
> + *
> + * SECTION: Structs and Unions/Structs
> + */
> +
> +#pragma D option quiet
> +
> +/*
> + * Declare some structs, including a struct of structs.
> + */
> +
> +struct coords {
> + int x, y;
> +};
> +struct particle {
> + struct coords position;
> + struct coords velocity;
> +};
> +
> +/*
> + * Declare some global (uppercase) and clause-local (lowercase) variables.
> + * In each trio:
> + * - The first variable is ignored;
> + * it is declared just so we can test nonzero offsets.
> + * - The second variable is initialized.
> + * - The third variable is assigned to.
> + */
> + int K, L, M;
> +this int k, l, m;
> + struct coords P, Q, R;
> +this struct coords p, q, r;
> + struct particle A, B, C;
> +this struct particle a, b, c;
> +
> +BEGIN {
> + /* Initialize the second variables. */
> + L = 11;
> + this->l = 12;
> + Q.x = 13;
> + Q.y = 14;
> + this->q.x = 15;
> + this->q.y = 16;
> + B.position.x = 17;
> + B.position.y = 18;
> + B.velocity.x = 19;
> + B.velocity.y = 20;
> + this->b.position.x = 21;
> + this->b.position.y = 22;
> + this->b.velocity.x = 23;
> + this->b.velocity.y = 24;
> +
> + printf("%d %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
> + L ,
> + this->l ,
> + Q.x ,
> + Q.y ,
> + this->q.x ,
> + this->q.y ,
> + B.position.x,
> + B.position.y,
> + B.velocity.x,
> + B.velocity.y,
> + this->b.position.x,
> + this->b.position.y,
> + this->b.velocity.x,
> + this->b.velocity.y);
> +
> + /* assign scalars */
> +
> + M = this->l + 1000;
> + this->m = Q.x + 1100;
> + R.x = Q.y + 1200;
> + R.y = this->q.x + 1300;
> + this->r.x = this->q.y + 1400;
> + this->r.y = B.velocity.x + 1500;
> + C.velocity.x = B.velocity.y + 1600;
> + C.velocity.y = this->b.velocity.x + 1700;
> + this->c.velocity.x = this->b.velocity.y + 1800;
> + this->c.velocity.y = L + 1900;
> +
> + printf("%d %d %d %d %d %d %d %d %d %d\n",
> + M ,
> + this->m ,
> + R.x ,
> + R.y ,
> + this->r.x ,
> + this->r.y ,
> + C.velocity.x,
> + C.velocity.y,
> + this->c.velocity.x,
> + this->c.velocity.y);
> +
> + /* assign struct (global-to-global or local-to-local) */
> +
> + R = Q;
> + this->r = this->q;
> + C.velocity = B.velocity;
> + this->c.velocity = this->b.velocity;
> +
> + printf("%d %d %d %d %d %d %d %d\n",
> + R.x,
> + R.y,
> + this->r.x,
> + this->r.y,
> + C.velocity.x,
> + C.velocity.y,
> + this->c.velocity.x,
> + this->c.velocity.y);
> +
> + /* assign struct (global-to-local or local-to-global) */
> +
> + R = this->q;
> + this->r = B.velocity;
> + C.velocity = this->b.velocity;
> + this->c.velocity = Q;
> +
> + printf("%d %d %d %d %d %d %d %d\n",
> + R.x,
> + R.y,
> + this->r.x,
> + this->r.y,
> + C.velocity.x,
> + C.velocity.y,
> + this->c.velocity.x,
> + this->c.velocity.y);
> +
> + /* assign struct of structs (global-to-global or local-to-local) */
> +
> + C = B;
> + this->c = this->b;
> +
> + printf("%d %d %d %d %d %d %d %d\n",
> + C.position.x, C.position.y,
> + C.velocity.x, C.velocity.y,
> + this->c.position.x, this->c.position.y,
> + this->c.velocity.x, this->c.velocity.y);
> +
> + /* assign struct of structs (global-to-local or local-to-global) */
> +
> + C = this->b;
> + this->c = B;
> +
> + printf("%d %d %d %d %d %d %d %d\n",
> + C.position.x, C.position.y,
> + C.velocity.x, C.velocity.y,
> + this->c.position.x, this->c.position.y,
> + this->c.velocity.x, this->c.velocity.y);
> +
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
This really ought to be a couple of tests. As far as I can see, each comment
that starts with 'assign' pretty much indicates another unittest and therefore
ought to be in its own file. That also makes identifying failures a lot
easier (you can see which case is failing just from the runtest.sum).
> diff --git a/test/unittest/struct/tst.global_clauselocal.r b/test/unittest/struct/tst.global_clauselocal.r
> new file mode 100644
> index 00000000..ef960c24
> --- /dev/null
> +++ b/test/unittest/struct/tst.global_clauselocal.r
> @@ -0,0 +1,7 @@
> +11 12 13 14 15 16 17 18 19 20 21 22 23 24
> +1012 1113 1214 1315 1416 1519 1620 1723 1824 1911
> +13 14 15 16 19 20 23 24
> +15 16 19 20 23 24 13 14
> +17 18 19 20 21 22 23 24
> +21 22 23 24 17 18 19 20
> +
With the tests split up, this will become multiple files as well.
> diff --git a/test/unittest/variables/lvar/tst.load_before_store.d b/test/unittest/variables/lvar/tst.load_before_store.d
> index 049719f3..98cdf120 100644
> --- a/test/unittest/variables/lvar/tst.load_before_store.d
> +++ b/test/unittest/variables/lvar/tst.load_before_store.d
> @@ -1,16 +1,13 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2020, 2021, 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: Test that a load-before-store pattern for a local variable forces
> - * the local variable to be initialized prior to loading.
> - *
> - * FAILURE: If the initialization is not done, the BPF verifier will reject
> - * the program due to reading from an uninitialized stack location.
> + * ASSERTION: Test that a load-before-store pattern for a local variable
> + * does not cause the script to fail.
> *
> * SECTION: Variables/Clause-Local Variables
> */
> diff --git a/test/unittest/variables/lvar/tst.struct.d b/test/unittest/variables/lvar/tst.struct.d
> index 3309b487..c9530caf 100644
> --- a/test/unittest/variables/lvar/tst.struct.d
> +++ b/test/unittest/variables/lvar/tst.struct.d
> @@ -1,10 +1,9 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2021, 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: Clause-local variables can be declared with a struct type.
> @@ -21,7 +20,7 @@ this struct foo x;
> BEGIN
> {
> this->x.a = 1;
> - this->x.b = 1;
> + this->x.b = 5;
>
> trace(this->x.a);
> trace(this->x.b);
> diff --git a/test/utils/print-stack-layout.c b/test/utils/print-stack-layout.c
> index bb5da3af..1b5cdd54 100644
> --- a/test/utils/print-stack-layout.c
> +++ b/test/utils/print-stack-layout.c
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2020, 2021, 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.
> */
> @@ -22,19 +22,7 @@ int main(void)
> printf("%%r6: % 4d\n", DT_STK_SPILL(6));
> printf("%%r7: % 4d\n", DT_STK_SPILL(7));
> printf("%%r8: % 4d\n", DT_STK_SPILL(8));
> - printf("lvar[% 3d]: % 4d (ID % 3d)\n", -1, DT_STK_LVAR(0) + 1,
> - DT_LVAR_OFF2ID(DT_STK_LVAR(0) + 1));
> - printf("lvar[% 3d]: % 4d (ID % 3d)\n", 0, DT_STK_LVAR(0),
> - DT_LVAR_OFF2ID(DT_STK_LVAR(0)));
> - printf("lvar[% 3d]: % 4d (ID % 3d)\n", 1, DT_STK_LVAR(1),
> - DT_LVAR_OFF2ID(DT_STK_LVAR(1)));
> - printf("lvar[% 3d]: % 4d (ID % 3d)\n", DT_LVAR_MAX,
> - DT_STK_LVAR(DT_LVAR_MAX),
> - DT_LVAR_OFF2ID(DT_STK_LVAR(DT_LVAR_MAX)));
> - printf("lvar[% 3d]: % 4d (ID % 3d)\n", -1,
> - DT_STK_LVAR(DT_LVAR_MAX) - 1,
> - DT_LVAR_OFF2ID(DT_STK_LVAR(DT_LVAR_MAX) - 1));
> - printf("scratch: % 4d .. % 4d\n", DT_STK_LVAR_END - 1,
> - DT_STK_SCRATCH_BASE);
> + printf("DT_STK_SCRATCH_BASE: % 4d\n", DT_STK_SCRATCH_BASE);
> + printf("DT_STK_SCRATCH_SZ: % 4d\n", DT_STK_SCRATCH_SZ);
This should be:
printf("scratch: % 4d .. % 4d\n",
DT_STK_SCRATCH_BASE + DT_STK_SCRATCH_SZ, DT_STK_SCRATCH_BASE);
> exit(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