[DTrace-devel] [PATCH] Add tests for clause-local variables
Eugene Loh
eugene.loh at oracle.com
Wed May 20 12:56:41 PDT 2020
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
> 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?
> + * 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.
> +
> 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
> *
> + * 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.
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.
> 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.
> 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
More information about the DTrace-devel
mailing list