[DTrace-devel] [PATCH 06/12] Assigning 0 to any dynamic variable should delete it
Kris Van Hees
kris.van.hees at oracle.com
Mon Jan 8 20:45:44 UTC 2024
On Mon, Jan 08, 2024 at 03:10:13PM -0500, Eugene Loh via DTrace-devel wrote:
> 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.
I can do that. It does not quite change the language to the extend that
assigning 0 to a dynamic variable is meant to delete the variable.
E.g. (from the docs):
"Conversely, assigning an associative array element to zero causes DTrace to
deallocate the underlying storage."
"Also, as with associative array elements, assigning zero to a thread-local
variable causes DTrace to deallocate the underlying storage. Always assign
zero to thread-local variables that are no longer in use."
So, DTtrace was actually not implementing what the documentation describes.
But I will explain that in the commit message so it is clear.
> 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.)
Well, this change only applies to dynamic variables, not to the general case
of struct = int or union = int.
> The tests have spaces rather than tabs for indenting struct/union decls?
Oops, will gix.
> s/associate/associative/ in the corresponding tests.
Thanks.
> 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.
Hm, that is a mistake - will change it (probably to 79 to be consistent).
> 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.
We already have those tests becuse the drop supoprt testing handles that.
> 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
>
> _______________________________________________
> 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