[DTrace-devel] [PATCH] Add tests for clause-local variables
Kris Van Hees
kris.van.hees at oracle.com
Wed May 20 13:21:08 PDT 2020
On Wed, May 20, 2020 at 12:56:41PM -0700, Eugene Loh wrote:
> Comments below, though perhaps these tests do not warrant much discussion.
>
>
> On 05/18/2020 10:44 AM, Kris Van Hees wrote:
>
> > This patch adds some tests for clause-local variables as well as moving
> > some existing tests into this specific collection of tests.
>
> Hmm, okay, but how about moving these as well?
> assocs/tst.diffprofile.d
> assocs/tst.this.d
Those will probably be moved at some later point as well (or be rewritten).
The goal here was not to go through the entire testsuite and pull out all the
tests that are specific to clause-local variables. Improving the testsuite
will be an ongoing effort.
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > test/unittest/assocs/tst.initialize.r | 2 --
> > .../lvar/err.D_OP_INCOMPAT.default_int.d | 22 ++++++++++++++
> > .../lvar/tst.default_int.d} | 12 ++++----
> > .../lvar/tst.init.d} | 13 +++-----
> > .../lvar/tst.load_before_store.d} | 5 +++-
> > .../lvar/tst.post_inc_lvar.d} | 2 ++
> > .../lvar/tst.post_inc_lvar.r} | 2 +-
> > .../lvar/tst.pre_inc_lvar.d} | 2 ++
> > .../lvar/tst.pre_inc_lvar.r} | 2 +-
> > test/unittest/variables/lvar/tst.struct.d | 30 +++++++++++++++++++
> > test/unittest/variables/lvar/tst.struct.r | 5 ++++
> > 11 files changed, 76 insertions(+), 21 deletions(-)
> > delete mode 100644 test/unittest/assocs/tst.initialize.r
> > create mode 100644 test/unittest/variables/lvar/err.D_OP_INCOMPAT.default_int.d
> > rename test/unittest/{scalars/tst.this.d => variables/lvar/tst.default_int.d} (61%)
> > rename test/unittest/{assocs/tst.initialize.d => variables/lvar/tst.init.d} (59%)
> > rename test/unittest/{codegen/tst.load_before_store_lvar.d => variables/lvar/tst.load_before_store.d} (79%)
> > rename test/unittest/{codegen/tst.post_inc_lvar_val.d => variables/lvar/tst.post_inc_lvar.d} (88%)
> > rename test/unittest/{codegen/tst.post_inc_lvar_val.r => variables/lvar/tst.post_inc_lvar.r} (55%)
> > rename test/unittest/{codegen/tst.pre_inc_lvar_val.d => variables/lvar/tst.pre_inc_lvar.d} (88%)
> > rename test/unittest/{codegen/tst.pre_inc_lvar_val.r => variables/lvar/tst.pre_inc_lvar.r} (55%)
> > create mode 100644 test/unittest/variables/lvar/tst.struct.d
> > create mode 100644 test/unittest/variables/lvar/tst.struct.r
> >
> > diff --git a/test/unittest/assocs/tst.initialize.r b/test/unittest/assocs/tst.initialize.r
> > deleted file mode 100644
> > index d460633e..00000000
> > --- a/test/unittest/assocs/tst.initialize.r
> > +++ /dev/null
> > @@ -1,2 +0,0 @@
> > -the value of x is 0
> > -
> > diff --git a/test/unittest/variables/lvar/err.D_OP_INCOMPAT.default_int.d b/test/unittest/variables/lvar/err.D_OP_INCOMPAT.default_int.d
> > new file mode 100644
> > index 00000000..81dc0720
> > --- /dev/null
> > +++ b/test/unittest/variables/lvar/err.D_OP_INCOMPAT.default_int.d
> > @@ -0,0 +1,22 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
>
> If this is a new file, why does it have a 2006 copyright? Because it
> was adopted from tst.default_int.d?
Yes.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +/*
> > + * ASSERTION: A clause-local variable declared without a type is implicitly
> > + * declared as an int. Verify that assignment of a string value
> > + * fails.
> > + *
> > + * SECTION: Variables/Clause-Local Variables
> > + */
> > +
> > +this x;
> > +
> > +BEGIN
> > +{
> > + this->x = execname;
> > + exit(0);
> > +}
> > diff --git a/test/unittest/scalars/tst.this.d b/test/unittest/variables/lvar/tst.default_int.d
> > similarity index 61%
> > rename from test/unittest/scalars/tst.this.d
> > rename to test/unittest/variables/lvar/tst.default_int.d
> > index d313a8c9..d616721a 100644
> > --- a/test/unittest/scalars/tst.this.d
> > +++ b/test/unittest/variables/lvar/tst.default_int.d
> > @@ -4,20 +4,18 @@
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > * http://oss.oracle.com/licenses/upl.
> > */
> > -/* @@xfail: dtv2 */
> >
> > /*
> > - * ASSERTION:
> > - *
> > - * Simple 'this' declaration.
> > - *
> > - * SECTION: Variables/Scalar Variables
> > + * ASSERTION: A clause-local variable declared without a type is implicitly
> > + * declared as an int.
> > *
> > + * SECTION: Variables/Clause-Local Variables
> > */
>
> To me, the point of this test is, to put it mildly, rather subtle. I
> think it'd be useful to add some comment about the intended scope of
> this "unit test" so that it is preserved by future developers. E.g., it
> may be helpful to refer to err.*.default_int.d since looking at the two
> tests side-by-side makes it quite a bit clearer what is intended. IMHO.
I think that the assertion makes it quite clear what is being tested here.
Both tests having 'default_int' as identifier in their name also makes it
quite clear they are related.
> > +
> > this x;
> >
> > BEGIN
> > {
> > - x = "dummy";
> > + x = 1;
> > exit(0);
> > }
> > diff --git a/test/unittest/assocs/tst.initialize.d b/test/unittest/variables/lvar/tst.init.d
> > similarity index 59%
> > rename from test/unittest/assocs/tst.initialize.d
> > rename to test/unittest/variables/lvar/tst.init.d
> > index 0db2c763..58d2e434 100644
> > --- a/test/unittest/assocs/tst.initialize.d
> > +++ b/test/unittest/variables/lvar/tst.init.d
> > @@ -4,16 +4,12 @@
> > * Licensed under the Universal Permissive License v 1.0 as shown at
> > * http://oss.oracle.com/licenses/upl.
> > */
> > -/* @@xfail: dtv2 */
> >
> > /*
> > - * ASSERTION:
> > - *
> > - * Clause local variables are not initialized to zero.
> > - *
> > - * SECTION: Variables/Associative Arrays
> > - *
> > + * ASSERTION: Clause local variables are initialized to zero. THis test will
> > + * report failure when 'x' is not zero.
>
> THis -> This
>
> More importantly, however, how is "not zero" a test failure? The
> documented behavior is that a clause-local variable is undefined upon
> entry.
> https://docs.oracle.com/en/operating-systems/oracle-linux/dtrace-guide/dt_clocal_dlang.html
The documented behaviour is not maintainable for DTrace v2 and given that it
stated that clause-local variables are undefined upon entry, being set to zero
is valid within that context. For DTrace v2 we need to ensure that there is an
explicitly set value for clause-local variables and 0 is the most reasonable
initial value. Therefore, this test exists to ensure that the initial value is
indeed 0.
The documentation will change in the future, but again, given that the initial
value is documented as undefined right now, it being 0 is not in violation of
the documentation.
> > *
> > + * SECTION: Variables/Clause-Local Variables
> > */
> >
> > #pragma D option quiet
> > @@ -22,6 +18,5 @@ this int x;
> >
> > BEGIN
> > {
> > - printf("the value of x is %d\n", this->x);
> > - exit(0);
> > + exit(this->x);
> > }
> > diff --git a/test/unittest/codegen/tst.load_before_store_lvar.d b/test/unittest/variables/lvar/tst.load_before_store.d
> > similarity index 79%
> > rename from test/unittest/codegen/tst.load_before_store_lvar.d
> > rename to test/unittest/variables/lvar/tst.load_before_store.d
> > index 12bb3565..049719f3 100644
> > --- a/test/unittest/codegen/tst.load_before_store_lvar.d
> > +++ b/test/unittest/variables/lvar/tst.load_before_store.d
> > @@ -8,8 +8,11 @@
> > /*
> > * 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.
> > + * the program due to reading from an uninitialized stack location.
> > + *
> > + * SECTION: Variables/Clause-Local Variables
> > */
> >
> > BEGIN
> > diff --git a/test/unittest/codegen/tst.post_inc_lvar_val.d b/test/unittest/variables/lvar/tst.post_inc_lvar.d
> > similarity index 88%
> > rename from test/unittest/codegen/tst.post_inc_lvar_val.d
> > rename to test/unittest/variables/lvar/tst.post_inc_lvar.d
> > index d340e578..5779f489 100644
> > --- a/test/unittest/codegen/tst.post_inc_lvar_val.d
> > +++ b/test/unittest/variables/lvar/tst.post_inc_lvar.d
> > @@ -8,6 +8,8 @@
> > /*
> > * ASSERTION: Test that post-increment on an local variable evaluates to the
> > * old value of the local variable.
> > + *
> > + * SECTION: Variables/Clause-Local Variables
> > */
> >
> > BEGIN
>
> Why not use the same methodology as for tst.init.d? That is, no trace()
> action; simply exit(this->x++) and skip the .r file.
Because I do care to know what the value is. If e.g. the code to ensure that
the initial value of a clause-local variable is set to 0 is not working, there
is no need for this test to fail as long as it does what it is testing.
> Also, FWIW, this doesn't seem like a variable test. It's more of a
> post-fix test, in which one should also test that the postfix did something.
The code generation is specific to the type of variable being used. We are
not (in this test) interested in ensuring that the postfix performed the
increment - we are making sure that the value of the postfix expression is the
old value of the variable.
> > diff --git a/test/unittest/codegen/tst.post_inc_lvar_val.r b/test/unittest/variables/lvar/tst.post_inc_lvar.r
> > similarity index 55%
> > rename from test/unittest/codegen/tst.post_inc_lvar_val.r
> > rename to test/unittest/variables/lvar/tst.post_inc_lvar.r
> > index 70eb9da6..2365ba1e 100644
> > --- a/test/unittest/codegen/tst.post_inc_lvar_val.r
> > +++ b/test/unittest/variables/lvar/tst.post_inc_lvar.r
> > @@ -2,4 +2,4 @@
> > :BEGIN 0
> >
> > -- @@stderr --
> > -dtrace: script 'test/unittest/codegen/tst.post_inc_lvar_val.d' matched 1 probe
> > +dtrace: script 'test/unittest/variables/lvar/tst.post_inc_lvar.d' matched 1 probe
> > diff --git a/test/unittest/codegen/tst.pre_inc_lvar_val.d b/test/unittest/variables/lvar/tst.pre_inc_lvar.d
> > similarity index 88%
> > rename from test/unittest/codegen/tst.pre_inc_lvar_val.d
> > rename to test/unittest/variables/lvar/tst.pre_inc_lvar.d
> > index 65a2ecfc..8c25baf1 100644
> > --- a/test/unittest/codegen/tst.pre_inc_lvar_val.d
> > +++ b/test/unittest/variables/lvar/tst.pre_inc_lvar.d
> > @@ -8,6 +8,8 @@
> > /*
> > * ASSERTION: Test that pre-increment on an local variable evaluates to the
> > * new value of the local variable.
> > + *
> > + * SECTION: Variables/Clause-Local Variables
> > */
> >
> > BEGIN
>
> Again, FWIW, feels more like an arithmetic test than a variable test.
Same comment as above.
> > diff --git a/test/unittest/codegen/tst.pre_inc_lvar_val.r b/test/unittest/variables/lvar/tst.pre_inc_lvar.r
> > similarity index 55%
> > rename from test/unittest/codegen/tst.pre_inc_lvar_val.r
> > rename to test/unittest/variables/lvar/tst.pre_inc_lvar.r
> > index e6166076..75fb99d6 100644
> > --- a/test/unittest/codegen/tst.pre_inc_lvar_val.r
> > +++ b/test/unittest/variables/lvar/tst.pre_inc_lvar.r
> > @@ -2,4 +2,4 @@
> > :BEGIN 1
> >
> > -- @@stderr --
> > -dtrace: script 'test/unittest/codegen/tst.pre_inc_lvar_val.d' matched 1 probe
> > +dtrace: script 'test/unittest/variables/lvar/tst.pre_inc_lvar.d' matched 1 probe
> > diff --git a/test/unittest/variables/lvar/tst.struct.d b/test/unittest/variables/lvar/tst.struct.d
> > new file mode 100644
> > index 00000000..3309b487
> > --- /dev/null
> > +++ b/test/unittest/variables/lvar/tst.struct.d
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2006, 2020, 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.
> > + *
> > + * SECTION: Variables/Clause-Local Variables
> > + */
> > +
> > +struct foo {
> > + int a;
> > + int b;
> > +};
> > +this struct foo x;
> > +
> > +BEGIN
> > +{
> > + this->x.a = 1;
> > + this->x.b = 1;
> > +
> > + trace(this->x.a);
> > + trace(this->x.b);
> > +
> > + exit(this->x.a == 1 && this->x.b == 5 ? 0 : 1);
> > +}
> > diff --git a/test/unittest/variables/lvar/tst.struct.r b/test/unittest/variables/lvar/tst.struct.r
> > new file mode 100644
> > index 00000000..be8bbc60
> > --- /dev/null
> > +++ b/test/unittest/variables/lvar/tst.struct.r
> > @@ -0,0 +1,5 @@
> > + FUNCTION:NAME
> > + :BEGIN 1 5
> > +
> > +-- @@stderr --
> > +dtrace: script 'test/unittest/variables/lvar/tst.struct.d' matched 1 probe
>
>
> _______________________________________________
> 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