[DTrace-devel] [PATCH 06/12] Assigning 0 to any dynamic variable should delete it
Eugene Loh
eugene.loh at oracle.com
Mon Jan 8 20:10:13 UTC 2024
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
But my leading question is whether this changes the D programming
language. E.g., I assume the associated scripts won't run with legacy
DTrace? At the very least, this change should be mentioned in the
commit message.
Related to that, I'm surprised this change doesn't break some test
designed to catch "struct = int" errors. (Or maybe not surprised if our
test coverage was weak. But then maybe this patch should introduce such
a test, where "struct = 0" passes through.)
The tests have spaces rather than tabs for indenting struct/union decls?
s/associate/associative/ in the corresponding tests.
The assoc array tests have room for four dvars. They should have room
for *three* dvars? So dynvarsize=60 rather than 80. With 80, the
"delete" could be doing nothing.
I guess one way to catch such a test mistake would be to add another set
of tests showing that without the delete things don't work? That is,
omit the "delete" statement and show that something is dropped. That
would serve as a baseline for the test that shows that the delete does
work. Maybe overkill. Maybe it suffices simply to get the test right.
But such a baseline would be helpful if, e.g., some day dynvarsize
changes behavior.
On 1/5/24 00:29, Kris Van Hees via DTrace-devel wrote:
> Assigning 0 to a scalar dynamic variable deletes it from storage, but
> due to type compatibility rules, this cannot be used to delete dynamic
> variables of struct of union type.
"struct of union"
->
"struct or union"
> Add a special case for assigning a literal 0 constant value to any
> dynamic variable to signify deleting the variable from storage.
>
> Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
> ---
> libdtrace/dt_cg.c | 18 +++++--
> libdtrace/dt_parser.c | 12 ++++-
> .../assocs/tst.store_zero_deletes_struct.d | 50 +++++++++++++++++++
> .../assocs/tst.store_zero_deletes_struct.r | 1 +
> .../assocs/tst.store_zero_deletes_union.d | 50 +++++++++++++++++++
> .../assocs/tst.store_zero_deletes_union.r | 1 +
> .../tvar/tst.store_zero_deletes_struct.d | 42 ++++++++++++++++
> .../tvar/tst.store_zero_deletes_struct.r | 1 +
> .../tvar/tst.store_zero_deletes_union.d | 42 ++++++++++++++++
> .../tvar/tst.store_zero_deletes_union.r | 1 +
> 10 files changed, 214 insertions(+), 4 deletions(-)
> create mode 100644 test/unittest/assocs/tst.store_zero_deletes_struct.d
> create mode 100644 test/unittest/assocs/tst.store_zero_deletes_struct.r
> create mode 100644 test/unittest/assocs/tst.store_zero_deletes_union.d
> create mode 100644 test/unittest/assocs/tst.store_zero_deletes_union.r
> create mode 100644 test/unittest/variables/tvar/tst.store_zero_deletes_struct.d
> create mode 100644 test/unittest/variables/tvar/tst.store_zero_deletes_struct.r
> create mode 100644 test/unittest/variables/tvar/tst.store_zero_deletes_union.d
> create mode 100644 test/unittest/variables/tvar/tst.store_zero_deletes_union.r
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index d9d56f0a..615ebdbf 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3627,15 +3627,26 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>
> /* First handle dvars. */
> if (idp->di_kind == DT_IDENT_ARRAY || idp->di_flags & DT_IDFLG_TLS) {
> - uint_t lbl_done = dt_irlist_label(dlp);
> -
> - uint_t lbl_notnull = dt_irlist_label(dlp);
> + uint_t lbl_done = dt_irlist_label(dlp);
> + uint_t lbl_notnull = dt_irlist_label(dlp);
> + dt_node_t *rp = dnp->dn_right;
>
> dt_cg_prep_dvar_args(dnp, dlp, drp, idp, 1, &fnp);
>
> dt_regset_xalloc(drp, BPF_REG_0);
> emite(dlp, BPF_CALL_FUNC(fnp->di_id), fnp);
> dt_regset_free_args(drp);
> +
> + /*
> + * Special case: a store of a literal 0 causes the dynamic
> + * variable to be deleted by the BPF function called above, so
> + * nothing is left to be done.
> + */
> + if (rp->dn_kind == DT_NODE_INT && rp->dn_value == 0) {
> + dt_regset_free(drp, BPF_REG_0);
> + goto dvar_deleted;
> + }
> +
> emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, dnp->dn_reg, 0, lbl_done));
> emit(dlp, BPF_BRANCH_IMM(BPF_JNE, BPF_REG_0, 0, lbl_notnull));
> emit(dlp, BPF_MOV_IMM(BPF_REG_0, 0));
> @@ -3669,6 +3680,7 @@ dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> emitl(dlp, lbl_done,
> BPF_NOP());
>
> +dvar_deleted:
> TRACE_REGSET(" store_var: End ");
>
> return;
> diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> index f6addc78..92580f1d 100644
> --- a/libdtrace/dt_parser.c
> +++ b/libdtrace/dt_parser.c
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2024, 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.
> */
> @@ -3791,6 +3791,16 @@ dt_cook_op2(dt_node_t *dnp, uint_t idflags)
> if (dt_node_is_argcompat(lp, rp))
> goto asgn_common;
>
> + /*
> + * Special case: assigning a literal 0 is allowed for dynamic
> + * variables because that is used to delete them from storage.
> + */
> + if (lp->dn_kind == DT_NODE_VAR &&
> + (lp->dn_ident->di_kind == DT_IDENT_ARRAY ||
> + (lp->dn_ident->di_flags & DT_IDFLG_TLS)) &&
> + rp->dn_kind == DT_NODE_INT && rp->dn_value == 0)
> + goto asgn_common;
> +
> xyerror(D_OP_INCOMPAT,
> "operands have incompatible types: \"%s\" %s \"%s\"\n",
> dt_node_type_name(lp, n1, sizeof(n1)), opstr(op),
> diff --git a/test/unittest/assocs/tst.store_zero_deletes_struct.d b/test/unittest/assocs/tst.store_zero_deletes_struct.d
> new file mode 100644
> index 00000000..11dbd1ba
> --- /dev/null
> +++ b/test/unittest/assocs/tst.store_zero_deletes_struct.d
> @@ -0,0 +1,50 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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, even for structs.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +/*
> + * We need enough storage to store 4 dynamic variables. Each dynamic variable
> + * will require 20 bytes of storage:
> + * 0..3 = Variable ID
> + * 4..7 = Index
> + * 8..15 = 0
> + * 16..19 = Value
> + */
> +#pragma D option dynvarsize=80
> +#pragma D option quiet
> +
> +this struct {
> + int a;
> +} val;
> +
> +BEGIN
> +{
> + this->val.a = 1;
> + a[1] = this->val;
> + this->val.a = 2;
> + a[2] = this->val;
> + this->val.a = 3;
> + a[3] = this->val;
> + a[1] = 0;
> + this->val.a = 4;
> + a[4] = this->val;
> + trace(a[2].a);
> + trace(a[3].a);
> + trace(a[4].a);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/assocs/tst.store_zero_deletes_struct.r b/test/unittest/assocs/tst.store_zero_deletes_struct.r
> new file mode 100644
> index 00000000..7b5813c6
> --- /dev/null
> +++ b/test/unittest/assocs/tst.store_zero_deletes_struct.r
> @@ -0,0 +1 @@
> +234
> diff --git a/test/unittest/assocs/tst.store_zero_deletes_union.d b/test/unittest/assocs/tst.store_zero_deletes_union.d
> new file mode 100644
> index 00000000..3d43f373
> --- /dev/null
> +++ b/test/unittest/assocs/tst.store_zero_deletes_union.d
> @@ -0,0 +1,50 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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, even for unions.
> + *
> + * SECTION: Variables/Associative Arrays
> + */
> +
> +/*
> + * We need enough storage to store 4 dynamic variables. Each dynamic variable
> + * will require 20 bytes of storage:
> + * 0..3 = Variable ID
> + * 4..7 = Index
> + * 8..15 = 0
> + * 16..19 = Value
> + */
> +#pragma D option dynvarsize=80
> +#pragma D option quiet
> +
> +this union {
> + int a;
> +} val;
> +
> +BEGIN
> +{
> + this->val.a = 1;
> + a[1] = this->val;
> + this->val.a = 2;
> + a[2] = this->val;
> + this->val.a = 3;
> + a[3] = this->val;
> + a[1] = 0;
> + this->val.a = 4;
> + a[4] = this->val;
> + trace(a[2].a);
> + trace(a[3].a);
> + trace(a[4].a);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/assocs/tst.store_zero_deletes_union.r b/test/unittest/assocs/tst.store_zero_deletes_union.r
> new file mode 100644
> index 00000000..7b5813c6
> --- /dev/null
> +++ b/test/unittest/assocs/tst.store_zero_deletes_union.r
> @@ -0,0 +1 @@
> +234
> diff --git a/test/unittest/variables/tvar/tst.store_zero_deletes_struct.d b/test/unittest/variables/tvar/tst.store_zero_deletes_struct.d
> new file mode 100644
> index 00000000..fc73b3b9
> --- /dev/null
> +++ b/test/unittest/variables/tvar/tst.store_zero_deletes_struct.d
> @@ -0,0 +1,42 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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 a TLS variable removes it from storage, making
> + * room for another variable, even if it is a struct.
> + *
> + * SECTION: Variables/Thread-Local Variables
> + */
> +
> +#pragma D option quiet
> +#pragma D option dynvarsize=15
> +
> +this struct {
> + int a;
> +} val;
> +
> +BEGIN
> +{
> + this->val.a = 1;
> + self->a = this->val;
> + this->val.a = 2;
> + self->b = this->val;
> + this->val.a = 3;
> + self->c = this->val;
> + self->a = 0;
> + this->val.a = 4;
> + self->d = this->val;
> + trace(self->b.a);
> + trace(self->c.a);
> + trace(self->d.a);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/variables/tvar/tst.store_zero_deletes_struct.r b/test/unittest/variables/tvar/tst.store_zero_deletes_struct.r
> new file mode 100644
> index 00000000..7b5813c6
> --- /dev/null
> +++ b/test/unittest/variables/tvar/tst.store_zero_deletes_struct.r
> @@ -0,0 +1 @@
> +234
> diff --git a/test/unittest/variables/tvar/tst.store_zero_deletes_union.d b/test/unittest/variables/tvar/tst.store_zero_deletes_union.d
> new file mode 100644
> index 00000000..8abb851f
> --- /dev/null
> +++ b/test/unittest/variables/tvar/tst.store_zero_deletes_union.d
> @@ -0,0 +1,42 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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 a TLS variable removes it from storage, making
> + * room for another variable, even if it is a union.
> + *
> + * SECTION: Variables/Thread-Local Variables
> + */
> +
> +#pragma D option quiet
> +#pragma D option dynvarsize=15
> +
> +this union {
> + int a;
> +} val;
> +
> +BEGIN
> +{
> + this->val.a = 1;
> + self->a = this->val;
> + this->val.a = 2;
> + self->b = this->val;
> + this->val.a = 3;
> + self->c = this->val;
> + self->a = 0;
> + this->val.a = 4;
> + self->d = this->val;
> + trace(self->b.a);
> + trace(self->c.a);
> + trace(self->d.a);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/variables/tvar/tst.store_zero_deletes_union.r b/test/unittest/variables/tvar/tst.store_zero_deletes_union.r
> new file mode 100644
> index 00000000..7b5813c6
> --- /dev/null
> +++ b/test/unittest/variables/tvar/tst.store_zero_deletes_union.r
> @@ -0,0 +1 @@
> +234
> -- 2.42.0
More information about the DTrace-devel
mailing list