[DTrace-devel] [PATCH 1/6] Change storage for global variables

Kris Van Hees kris.van.hees at oracle.com
Wed Mar 31 10:27:28 PDT 2021


Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>

... with changes as indicated below

I am a bit concerned that the fact that we're needing to load the gvars
pointer using two instructions for every access (get or set) for a global
variable.  We may need to look at a way to optimize that, but that is
definitely for future work.  Since I am planning on looking at an optimizer
stage in the compilation anyway, that may be a good way to tackle this.
Otherwise, we can look at making changes in the code gneeration.

On Fri, Mar 19, 2021 at 01:45:29PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Change how global variables are stored and accessed.  Instead of
> storing them as 8-byte quantities addressed by a variable's ID, just
> use a single-key BPF map addressed by a variable's offset.  This allows

"use the value of a single-key BPF map addressed"

> us to store global variables of "arbitrary" size (e.g., > 8 bytes).
> 
> A pointer to the BPF map is kept on the stack in the DTrace context.

"to the BPF map value"

> Since no more BPF helper function is used to access gvars, get rid of
> our gvar BPF functions.

Perhaps rephrase as:
The get_gvar and set_gvar BPF helper functions are now obsolete.

> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  BPF-NOTES                                     | 25 ++----
>  bpf/Build                                     |  1 -
>  bpf/get_gvar.c                                | 21 -----
>  bpf/map_gvar.c                                | 14 ---
>  bpf/set_gvar.c                                | 18 ----
>  libdtrace/dt_bpf.c                            | 14 ++-
>  libdtrace/dt_cg.c                             | 87 ++++++++++++-------
>  libdtrace/dt_dctx.h                           |  2 +
>  libdtrace/dt_dis.c                            | 12 ---
>  libdtrace/dt_dlibs.c                          |  2 -
>  libdtrace/dt_parser.c                         | 10 ++-
>  .../arrays/tst.declared-bounds.assignment.d   |  6 +-
>  .../inline/tst.InlineWritableAssign.d         |  3 +-
>  test/unittest/sizeof/tst.SizeofArray.d        |  3 +-
>  test/unittest/struct/tst.StructDataTypes.d    |  3 +-
>  .../translators/tst.CircularTransDecl.d       |  3 +-
>  .../translators/tst.PartialDereferencing.d    |  3 +-
>  .../translators/tst.TransNonPointer.d         |  3 +-
>  .../translators/tst.TransOutputPointer.d      |  3 +-
>  test/unittest/typedef/tst.TypedefDataAssign.d |  3 +-
>  test/unittest/union/tst.UnionDataTypes.d      |  3 +-
>  test/unittest/union/tst.UnionInside.d         |  3 +-
>  22 files changed, 94 insertions(+), 148 deletions(-)
>  delete mode 100644 bpf/get_gvar.c
>  delete mode 100644 bpf/map_gvar.c
>  delete mode 100644 bpf/set_gvar.c
> 
> diff --git a/BPF-NOTES b/BPF-NOTES
> index a0a40490..27c2900e 100644
> --- a/BPF-NOTES
> +++ b/BPF-NOTES
> @@ -6,7 +6,7 @@
>    list of probes we need (created perf events), enable them, and then flip
>    the master switch.
>  
> -- A usueful feature (that I think current DTrace cannot even do - need to
> +- A useful feature (that I think current DTrace cannot even do - need to
>    check) is listing which executable that are currently running on the system
>    have USDT support.
>  
> @@ -42,23 +42,16 @@
>    This means that in its most basic form, local variable i can be loaded from
>    [%fp-(i*8)-8].
>  
> -- Global variables can be stored in an array map, indexed by the variable id,
> -  and with a u64 value.  Variables that store data items that are larger than
> -  64-bits will store an offset into a memory block obtained as a map value from
> -  an array map.
> +- 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:
>  
> -  Due to the requirement that key and value are to be on the stack for map
> -  updates, we need to generate code that does the following:
> +	- load the DTrace context address (on the stack)
> +	- dereference at the right offset for the map pointer

"map value pointer"

> +	- dereference usin the variable's offset

"using"

> -	- Store global variable id in stack slot
> -	- Store value in stack slot
> -	- Set up arguments for bpf_map_update_elem helper call
> -	- Call bpf_map_update_elem helper
> -
> -  It might be a good idea to create a set_gvar(id, val) eBPF function that is
> -  called from the compiled D code to save some instructions (as opposed to
> -  generating the instruction sequence shown above for every global variable
> -  store).
> +  No function calls are needed (whether to our function or a BPF helper
> +  function), and so these few instructions can be generated each time a
> +  global variable is accessed.
>  
>  - In the new design, there is no need for shiting variable ids up beause of the
>    range of built-in variables because we can generate code to obtain the value
> diff --git a/bpf/Build b/bpf/Build
> index 36cf1fa0..f8d736e7 100644
> --- a/bpf/Build
> +++ b/bpf/Build
> @@ -24,7 +24,6 @@ bpf_dlib_SRCDEPS = $(objdir)/include/.dir.stamp
>  bpf_dlib_SOURCES = \
>  	agg_lqbin.c agg_qbin.c \
>  	get_bvar.c \
> -	get_gvar.c set_gvar.c \
>  	get_tvar.c set_tvar.c \
>  	probe_error.c \
>  	memcpy.c strnlen.c
> diff --git a/bpf/get_gvar.c b/bpf/get_gvar.c
> deleted file mode 100644
> index 59c4334d..00000000
> --- a/bpf/get_gvar.c
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.
> - */
> -#include <linux/bpf.h>
> -#include <stdint.h>
> -#include <bpf-helpers.h>
> -
> -#ifndef noinline
> -# define noinline	__attribute__((noinline))
> -#endif
> -
> -extern struct bpf_map_def gvars;
> -
> -noinline uint64_t dt_get_gvar(uint32_t id)
> -{
> -	uint64_t	*val;
> -
> -	val = bpf_map_lookup_elem(&gvars, &id);
> -	return val ? *val : 0;
> -}
> diff --git a/bpf/map_gvar.c b/bpf/map_gvar.c
> deleted file mode 100644
> index 1a943f3f..00000000
> --- a/bpf/map_gvar.c
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.
> - */
> -#include <linux/bpf.h>
> -#include <stdint.h>
> -#include <bpf-helpers.h>
> -
> -struct bpf_map_def SEC("maps") gvars = {
> -	.type = BPF_MAP_TYPE_ARRAY,
> -	.key_size = sizeof(uint32_t),
> -	.value_size = sizeof(uint64_t),
> -	.max_entries = 16,
> -};
> diff --git a/bpf/set_gvar.c b/bpf/set_gvar.c
> deleted file mode 100644
> index c1887eea..00000000
> --- a/bpf/set_gvar.c
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.
> - */
> -#include <linux/bpf.h>
> -#include <stdint.h>
> -#include <bpf-helpers.h>
> -
> -#ifndef noinline
> -# define noinline	__attribute__((noinline))
> -#endif
> -
> -extern struct bpf_map_def gvars;
> -
> -noinline void dt_set_gvar(uint32_t id, uint64_t val)
> -{
> -	bpf_map_update_elem(&gvars, &id, &val, BPF_ANY);
> -}
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 6637bdef..e33f6afd 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -171,10 +171,8 @@ set_task_offsets(dtrace_hdl_t *dtp)
>   *		concatenation of all unique strings (each terminated with a
>   *		NUL byte).  The string table size is taken from the DTrace
>   *		consumer handle (dt_strlen).
> - * - gvars:	Global variables map, associating a 64-bit value with each
> - *		global variable.  The map is indexed by global variable id.
> - *		The amount of global variables is the next-to--be-assigned
> - *		global variable id minus the base id.
> + * - gvars:	Global variables map.  This is a global 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
> @@ -197,7 +195,7 @@ set_task_offsets(dtrace_hdl_t *dtp)
>  int
>  dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>  {
> -	int		gvarc, tvarc, aggsz;
> +	int		gvarsz, tvarc, aggsz;
>  	int		ci_mapfd;
>  	uint32_t	key = 0;
>  
> @@ -212,7 +210,7 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>  	aggsz = dt_idhash_datasize(dtp->dt_aggs);
>  
>  	/* Determine the number of global and TLS variables. */
> -	gvarc = dt_idhash_peekid(dtp->dt_globals) - DIF_VAR_OTHER_UBASE;
> +	gvarsz = (dt_idhash_datasize(dtp->dt_globals) + 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. */
> @@ -254,9 +252,9 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>  			sizeof(uint32_t), dtp->dt_strlen, 1) == -1)
>  		return -1;		/* dt_errno is set for us */
>  
> -	if (gvarc > 0 &&
> +	if (gvarsz > 0 &&
>  	    create_gmap(dtp, "gvars", BPF_MAP_TYPE_ARRAY,
> -			sizeof(uint32_t), sizeof(uint64_t), gvarc) == -1)
> +			sizeof(uint32_t), gvarsz, 1) == -1)
>  		return -1;		/* dt_errno is set for us */
>  
>  	if (tvarc > 0 &&
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index c7c942d7..9243f0a7 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -137,31 +137,35 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
>  	emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 8));
>  	emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DCTX_FP(DCTX_BUF), BPF_REG_0));
>  
> -	if (dt_idhash_datasize(dtp->dt_aggs) > 0) {
> -		dt_ident_t	*aggs = dt_dlib_get_map(dtp, "aggs");
> -
> -		/*
> -		 * key = 0;		// stw [%fp + DCTX_FP(DCTX_AGG)], 0
> -		 * rc = bpf_map_lookup_elem(&aggs, &key);
> -		 *			// lddw %r1, &aggs
> -		 *			// mov %r2, %fp
> -		 *			// add %r2, DCTX_FP(DCTX_AGG)
> -		 *			// call bpf_map_lookup_elem
> -		 *			//     (%r1 ... %r5 clobbered)
> -		 *			//     (%r0 = 'aggs' BPF map value)
> -		 * if (rc == 0)		// jeq %r0, 0, lbl_exit
> -		 *	goto exit;
> -		 *			//     (%r0 = pointer to agg data)
> -		 * dctx.agg = rc;	// stdw [%fp + DCTX_FP(DCTX_AGG)], %r0
> -		 */
> -		emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_FP, DCTX_FP(DCTX_AGG), 0));
> -		dt_cg_xsetx(dlp, aggs, DT_LBL_NONE, BPF_REG_1, aggs->di_id);
> -		emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_FP));
> -		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DCTX_FP(DCTX_AGG)));
> -		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_map_lookup_elem));
> -		emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_exit));
> -		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DCTX_FP(DCTX_AGG), BPF_REG_0));
> -	}
> +	/*
> +	 * Store pointer to BPF map "name" in the DTrace context at "offset".
> +	 *     key = 0;                  // use address %fp + DCTX_FP(offset)
> +	 *     %r0 = bpf_map_lookup_elem(name, &key);
> +	 *     if (%r0 == 0)
> +	 *         goto exit;
> +	 *     [%fp + DCTX_FP(offset)] = %0
> +	 */

Please keep the original comment style, since there really is no need to change
it (aside from substituting generic names rather than specific names.

So, it would be:

	/*
	 * Store pointer to BPF map "name" in the DTrace context field "fld" at
	 * "offset".
	 *
	 * key = 0;		// stw [%fp + DCTX_FP(offset)], 0
	 * rc = bpf_map_lookup_elem(&name, &key);
	 *			// lddw %r1, &name
	 *			// mov %r2, %fp
	 *			// add %r2, DCTX_FP(offset)
	 *			// call bpf_map_lookup_elem
	 *			//     (%r1 ... %r5 clobbered)
	 *			//     (%r0 = name BPF map value)
	 * if (rc == 0)		// jeq %r0, 0, lbl_exit
	 *	goto exit;
	 *			//     (%r0 = pointer to map value)
	 * dctx.fld = rc;	// stdw [%fp + DCTX_FP(offset)], %r0
	 */

> +#define DT_CG_STORE_MAP_PTR(name, offset) \
> +	do { \
> +		dt_ident_t *idp = dt_dlib_get_map(dtp, name); \
> + \
> +		emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_FP, DCTX_FP(offset), 0)); \
> + \
> +		dt_cg_xsetx(dlp, idp, DT_LBL_NONE, BPF_REG_1, idp->di_id); \
> +		emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_FP)); \
> +		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DCTX_FP(offset))); \
> +		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_map_lookup_elem)); \
> + \
> +		emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_exit)); \
> + \
> +		emit(dlp, BPF_STORE(BPF_DW, BPF_REG_FP, DCTX_FP(offset), BPF_REG_0)); \
> +	} while(0)
> +
> +	if (dt_idhash_datasize(dtp->dt_aggs) > 0)
> +		DT_CG_STORE_MAP_PTR("aggs", DCTX_AGG);
> +	if (dt_idhash_datasize(dtp->dt_globals) > 0)
> +		DT_CG_STORE_MAP_PTR("gvars", DCTX_GVARS);
> +#undef DT_CG_STORE_MAP_PTR
>  }
>  
>  void
> @@ -1607,6 +1611,20 @@ dt_cg_load_var(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
>  		return;
>  	}
>  
> +	/* FIXME: need cleaner test for global var */
> +	if (!(idp->di_flags & DT_IDFLG_TLS) && idp->di_id >= DIF_VAR_OTHER_UBASE) {

I think we need to look at (possibly) introducing flags to distinguish between
the four different variable kinds: local, TLS, global, and built-in.  I know
that the built-in ones act as global variable when it comes to scope etc, but
the implementation is (of course) so different that using the *_UBASE barrier
on ID values has become silly.

But leave that for a later patch...  For now, let's just keep the FIXME in and
move on.

> +		if ((dst->dn_reg = dt_regset_alloc(drp)) == -1)
> +			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +		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));
> +		if ((dst->dn_flags & DT_NF_REF) == 0)

I'd prefer:

		if (!(dst->dn_flags & DT_NF_REF))

or perhaps even better, just reverse the two cases and get rid of the negation?
I.e. for pass-by-ref you do an ADD, and otherwise (most common case) you do a
LOAD.

> +			emit(dlp, BPF_LOAD(BPF_DW, dst->dn_reg, dst->dn_reg, idp->di_offset));
> +		else
> +			emit(dlp, BPF_ALU64_IMM(BPF_ADD, dst->dn_reg, idp->di_offset));
> +
> +		return;
> +	}
> +
>  	if (dt_regset_xalloc_args(drp) == -1)
>  		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>  
> @@ -1617,10 +1635,8 @@ dt_cg_load_var(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
>  		emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
>  		emit(dlp, BPF_MOV_IMM(BPF_REG_2, idp->di_id));
>  		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_bvar");
> -	} else {					/* global var */
> -		emit(dlp, BPF_MOV_IMM(BPF_REG_1, idp->di_id - DIF_VAR_OTHER_UBASE));
> -		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_gvar");
> -	}
> +	} else
> +		assert(0);
>  	assert(idp != NULL);
>  	dt_regset_xalloc(drp, BPF_REG_0);
>  	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> @@ -1896,8 +1912,17 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
>  
>  	if (idp->di_flags & DT_IDFLG_TLS)	/* TLS var */
>  		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_set_tvar");
> -	else					/* global var */
> -		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_set_gvar");
> +	else {					/* global var */
> +		int reg;
> +
> +		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));
> +		emit(dlp, BPF_STORE(BPF_DW, reg, idp->di_offset, src->dn_reg));
> +		dt_regset_free(drp, reg);
> +		return;
> +	}
>  
>  	assert(idp != NULL);
>  
> diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> index ec836b70..2707f9fd 100644
> --- a/libdtrace/dt_dctx.h
> +++ b/libdtrace/dt_dctx.h
> @@ -39,6 +39,7 @@ typedef struct dt_dctx {
>  	dt_mstate_t	*mst;		/* DTrace machine state */
>  	char		*buf;		/* Output buffer scratch memory */
>  	char		*agg;		/* Aggregation data */
> +	char		*gvars;		/* Global variables */
>  } dt_dctx_t;
>  
>  #define DCTX_CTX	offsetof(dt_dctx_t, ctx)
> @@ -46,6 +47,7 @@ typedef struct dt_dctx {
>  #define DCTX_MST	offsetof(dt_dctx_t, mst)
>  #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_SIZE	((int16_t)sizeof(dt_dctx_t))
>  
>  /*
> diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> index 3c8bd425..d5ea79ca 100644
> --- a/libdtrace/dt_dis.c
> +++ b/libdtrace/dt_dis.c
> @@ -254,18 +254,6 @@ dt_dis_bpf_args(const dtrace_difo_t *dp, const char *fn,
>  		snprintf(buf, len, "%s",
>  			 dt_dis_varname(dp, in->imm, DIFV_SCOPE_GLOBAL, addr));
>  		return buf;
> -	} else if (strcmp(fn, "dt_get_gvar") == 0 ||
> -		   strcmp(fn, "dt_set_gvar") == 0) {
> -		/*
> -		 * We know that the previous instruction exists and assigns
> -		 * the variable id to %r1 (because we wrote the code generator
> -		 * to emit these instructions in this exact order.)
> -		 */
> -		in--;
> -		snprintf(buf, len, "%s",
> -			 dt_dis_varname(dp, in->imm + DIF_VAR_OTHER_UBASE,
> -					DIFV_SCOPE_GLOBAL, addr));
> -		return buf;
>  	} else if (strcmp(fn, "dt_get_tvar") == 0 ||
>  		   strcmp(fn, "dt_set_tvar") == 0) {
>  		/*
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index 8536704d..2c293719 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -56,11 +56,9 @@ static const dt_ident_t		dt_bpf_symbols[] = {
>  	DT_BPF_SYMBOL(dt_agg_qbin, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_error, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_get_bvar, DT_IDENT_SYMBOL),
> -	DT_BPF_SYMBOL(dt_get_gvar, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_get_string, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_get_tvar, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_memcpy, DT_IDENT_SYMBOL),
> -	DT_BPF_SYMBOL(dt_set_gvar, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_set_tvar, DT_IDENT_SYMBOL),
>  	DT_BPF_SYMBOL(dt_strnlen, DT_IDENT_SYMBOL),
>  	/* BPF maps */
> diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> index c0301192..4971a4e2 100644
> --- a/libdtrace/dt_parser.c
> +++ b/libdtrace/dt_parser.c
> @@ -1,6 +1,6 @@
>  /*
>   * 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.
>   */
> @@ -1661,6 +1661,10 @@ dt_node_decl(void)
>  			if (idp == NULL)
>  				longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
>  
> +			/* FIXME: do not do this for aggregations; their size set in dt_cg.c */
> +			dt_ident_set_storage(idp, 8,
> +					     ctf_type_size(dtt.dtt_ctfp, type));
> +

I am confused... Does this mean that you are setting the storage size here but
really shouldn't be doing it?  If you shouldn't be doing this here for aggs,
then wouldn't you just put in a conditional clause for it?  That is what you
do in the next part of the patch...

>  			dt_ident_type_assign(idp, dtt.dtt_ctfp, dtt.dtt_type);
>  
>  			/*
> @@ -2751,6 +2755,10 @@ dt_xcook_ident(dt_node_t *dnp, dt_idhash_t *dhp, uint_t idkind, int create)
>  		if (idp == NULL)
>  			longjmp(yypcb->pcb_jmpbuf, EDT_NOMEM);
>  
> +		/* aggregation storage size is set later, in dt_cg.c */
> +		if (idkind != DT_IDENT_AGG)
> +			dt_ident_set_storage(idp, 8, 8);
> +
>  		/*
>  		 * Arrays and aggregations are not cooked individually. They
>  		 * have dynamic types and must be referenced using operator [].
> diff --git a/test/unittest/arrays/tst.declared-bounds.assignment.d b/test/unittest/arrays/tst.declared-bounds.assignment.d
> index b451a11a..336e341f 100644
> --- a/test/unittest/arrays/tst.declared-bounds.assignment.d
> +++ b/test/unittest/arrays/tst.declared-bounds.assignment.d
> @@ -1,14 +1,12 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2018, 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:
> - * 	Within-bounds array assignments work.
> + * ASSERTION: Within-bounds array assignments work.
>   *
>   * SECTION: Pointers and Arrays/Array Declarations and Storage
>   */
> diff --git a/test/unittest/inline/tst.InlineWritableAssign.d b/test/unittest/inline/tst.InlineWritableAssign.d
> index a939c9f7..ce95c025 100644
> --- a/test/unittest/inline/tst.InlineWritableAssign.d
> +++ b/test/unittest/inline/tst.InlineWritableAssign.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:
> diff --git a/test/unittest/sizeof/tst.SizeofArray.d b/test/unittest/sizeof/tst.SizeofArray.d
> index 7ad9784b..85e24c90 100644
> --- a/test/unittest/sizeof/tst.SizeofArray.d
> +++ b/test/unittest/sizeof/tst.SizeofArray.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: sizeof returns the size in bytes of any D expression or data
> diff --git a/test/unittest/struct/tst.StructDataTypes.d b/test/unittest/struct/tst.StructDataTypes.d
> index aef6247c..70a66ec8 100644
> --- a/test/unittest/struct/tst.StructDataTypes.d
> +++ b/test/unittest/struct/tst.StructDataTypes.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: Declaration of the different data types within a struct and
> diff --git a/test/unittest/translators/tst.CircularTransDecl.d b/test/unittest/translators/tst.CircularTransDecl.d
> index dda08c2a..9f921789 100644
> --- a/test/unittest/translators/tst.CircularTransDecl.d
> +++ b/test/unittest/translators/tst.CircularTransDecl.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:
> diff --git a/test/unittest/translators/tst.PartialDereferencing.d b/test/unittest/translators/tst.PartialDereferencing.d
> index 3d8c913d..e002e5b4 100644
> --- a/test/unittest/translators/tst.PartialDereferencing.d
> +++ b/test/unittest/translators/tst.PartialDereferencing.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:
> diff --git a/test/unittest/translators/tst.TransNonPointer.d b/test/unittest/translators/tst.TransNonPointer.d
> index 34be6189..c7f82c72 100644
> --- a/test/unittest/translators/tst.TransNonPointer.d
> +++ b/test/unittest/translators/tst.TransNonPointer.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:
> diff --git a/test/unittest/translators/tst.TransOutputPointer.d b/test/unittest/translators/tst.TransOutputPointer.d
> index 2ad03d2e..41cec485 100644
> --- a/test/unittest/translators/tst.TransOutputPointer.d
> +++ b/test/unittest/translators/tst.TransOutputPointer.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:
> diff --git a/test/unittest/typedef/tst.TypedefDataAssign.d b/test/unittest/typedef/tst.TypedefDataAssign.d
> index 2e77c1fe..df9d85aa 100644
> --- a/test/unittest/typedef/tst.TypedefDataAssign.d
> +++ b/test/unittest/typedef/tst.TypedefDataAssign.d
> @@ -1,11 +1,10 @@
>  /*
>   * 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 */
>  /* @@runtest-opts: -C */
>  
>  /*
> diff --git a/test/unittest/union/tst.UnionDataTypes.d b/test/unittest/union/tst.UnionDataTypes.d
> index 9b340232..aa8cc5ca 100644
> --- a/test/unittest/union/tst.UnionDataTypes.d
> +++ b/test/unittest/union/tst.UnionDataTypes.d
> @@ -1,6 +1,6 @@
>  /*
>   * 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.
>   */
> @@ -66,7 +66,6 @@ enum {
>  	unsigned long *pointer;
>  } var;
>  
> -/* @@xfail: dtv2 */
>  /* @@note: is this even supported? */
>  
>  /* var.pointer = &`max_pfn; */
> diff --git a/test/unittest/union/tst.UnionInside.d b/test/unittest/union/tst.UnionInside.d
> index ad5f5034..b23f2105 100644
> --- a/test/unittest/union/tst.UnionInside.d
> +++ b/test/unittest/union/tst.UnionInside.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:
> -- 
> 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