[DTrace-devel] [PATCH] test: Test disassembly annotation of variable access

Fri Jan 28 04:13:09 UTC 2022

On Thu, Jan 27, 2022 at 02:41:15PM -0500, Eugene Loh via DTrace-devel wrote:
> On 1/27/22 1:57 PM, Kris Van Hees wrote:
> 
> > Two things...
> > 
> > 1. Please update the tests to match the one I have for TLS vars in my patch
> >     (for consistency).
> 
> Yup.  Indeed, the tests are based off of yours.  So they are already very
> similar.  However,
> 
> *)  You look for a very distinctive BPF instruction:  "call dt_get_tvar". 
> This trick works for tvars, but not for other kinds of variables, which use
> rather general ld/st instructions.  So I rename the variable to something
> more distinctive and look for that distinctive variable name.
> 
> *)  You print out the entire BPF instruction.  This works for tvars, but not
> for other kinds of variables.  For lvars and gvars, there is no common
> function call (like dt_get_tvar).  Rather, there is a rather common BPF
> instruction (like ldw or stw) that references specific registers and
> specific offsets.  It seemed to me that we would not want to be overly
> dependent on details of code generation, test script, etc.  So I just print
> out the BPF operation, not including its operands.
> 
> *)  You only test "set".  Arguably, this is sufficient for tvars, where both
> "set" and "get" currently use dt_get_tvar().  For lvars and gvars, however,
> different instructions are generated for "set" and "get".  So I test both
> "set" and "get".

I apologize for my original review email having been a bit too brief.  I did
momt mean to imply that the tests you propose should be near identical to the
TLS Sone I included in my patch.  I meant to indicate that they should match
in functionality, i.e. ensure that the only valid matching instructions are
truly the ones that should have the annotation.  In the case of tvar that is
the call instruction.  In the case of gvar and lvar that is more complicated
because it depends on preceding instructions.  Function dt_dis_varname() in
dt_dis.c shows the sequences.

Your tests should ensure that the instructions you find the annotation on are
indeed supposed to have that annotation, i.e. not other store or load
instructions should bear the annotation.  And the tests should also cover the
case of access by reference (when the annotation is on the add instruction).

> So, the tests are indeed intended to match what you had, with variations
> introduced only to generalize beyond just the TLS case.
> 
> > 2. No need to do TLS vars (tvar) because I already have that one in my patch.
> 
> Like you, I figured consistency would be a good thing.  We can first settle
> the lvar/gvar cases and then decide whether tvar should be similar or
> different.

tvar already has a test - let's focus here on gvar and lvar.

> > On Thu, Jan 27, 2022 at 01:34:25PM -0500, eugene.loh--- via DTrace-devel wrote:
> > > From: Eugene Loh <eugene.loh at oracle.com>
> > > 
> > > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > > ---
> > >   test/unittest/variables/gvar/tst.dis-ann.r  |  2 ++
> > >   test/unittest/variables/gvar/tst.dis-ann.sh | 20 ++++++++++++++++++++
> > >   test/unittest/variables/lvar/tst.dis-ann.r  |  2 ++
> > >   test/unittest/variables/lvar/tst.dis-ann.sh | 20 ++++++++++++++++++++
> > >   test/unittest/variables/tvar/tst.dis-ann.r  |  2 ++
> > >   test/unittest/variables/tvar/tst.dis-ann.sh | 20 ++++++++++++++++++++
> > >   6 files changed, 66 insertions(+)
> > >   create mode 100644 test/unittest/variables/gvar/tst.dis-ann.r
> > >   create mode 100755 test/unittest/variables/gvar/tst.dis-ann.sh
> > >   create mode 100644 test/unittest/variables/lvar/tst.dis-ann.r
> > >   create mode 100755 test/unittest/variables/lvar/tst.dis-ann.sh
> > >   create mode 100644 test/unittest/variables/tvar/tst.dis-ann.r
> > >   create mode 100755 test/unittest/variables/tvar/tst.dis-ann.sh
> > > 
> > > diff --git a/test/unittest/variables/gvar/tst.dis-ann.r b/test/unittest/variables/gvar/tst.dis-ann.r
> > > new file mode 100644
> > > index 00000000..95586362
> > > --- /dev/null
> > > +++ b/test/unittest/variables/gvar/tst.dis-ann.r
> > > @@ -0,0 +1,2 @@
> > > +ldw foo
> > > +stw foo
> > > diff --git a/test/unittest/variables/gvar/tst.dis-ann.sh b/test/unittest/variables/gvar/tst.dis-ann.sh
> > > new file mode 100755
> > > index 00000000..291c64e8
> > > --- /dev/null
> > > +++ b/test/unittest/variables/gvar/tst.dis-ann.sh
> > > @@ -0,0 +1,20 @@
> > > +#!/bin/bash
> > > +#
> > > +# Oracle Linux DTrace.
> > > +# Copyright (c) 2022, 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.
> > > +#
> > > +
> > > +dtrace=$1
> > > +
> > > +$dtrace $dt_flags -Sen '
> > > +BEGIN
> > > +{
> > > +	foo = 42;
> > > +	trace(foo);
> > > +	exit(0);
> > > +}
> > > +' 2>&1 | awk '/foo/ && !/D type/ { print $8, $NF }'
> > > +
> > > +exit $?
> > > diff --git a/test/unittest/variables/lvar/tst.dis-ann.r b/test/unittest/variables/lvar/tst.dis-ann.r
> > > new file mode 100644
> > > index 00000000..a10af5a8
> > > --- /dev/null
> > > +++ b/test/unittest/variables/lvar/tst.dis-ann.r
> > > @@ -0,0 +1,2 @@
> > > +ldw this->foo
> > > +stw this->foo
> > > diff --git a/test/unittest/variables/lvar/tst.dis-ann.sh b/test/unittest/variables/lvar/tst.dis-ann.sh
> > > new file mode 100755
> > > index 00000000..bf3b9b83
> > > --- /dev/null
> > > +++ b/test/unittest/variables/lvar/tst.dis-ann.sh
> > > @@ -0,0 +1,20 @@
> > > +#!/bin/bash
> > > +#
> > > +# Oracle Linux DTrace.
> > > +# Copyright (c) 2022, 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.
> > > +#
> > > +
> > > +dtrace=$1
> > > +
> > > +$dtrace $dt_flags -Sen '
> > > +BEGIN
> > > +{
> > > +	this->foo = 42;
> > > +	trace(this->foo);
> > > +	exit(0);
> > > +}
> > > +' 2>&1 | awk '/foo/ && !/D type/ { print $8, $NF }'
> > > +
> > > +exit $?
> > > diff --git a/test/unittest/variables/tvar/tst.dis-ann.r b/test/unittest/variables/tvar/tst.dis-ann.r
> > > new file mode 100644
> > > index 00000000..f02effb1
> > > --- /dev/null
> > > +++ b/test/unittest/variables/tvar/tst.dis-ann.r
> > > @@ -0,0 +1,2 @@
> > > +call self->foo
> > > +call self->foo
> > > diff --git a/test/unittest/variables/tvar/tst.dis-ann.sh b/test/unittest/variables/tvar/tst.dis-ann.sh
> > > new file mode 100755
> > > index 00000000..9d28ab3b
> > > --- /dev/null
> > > +++ b/test/unittest/variables/tvar/tst.dis-ann.sh
> > > @@ -0,0 +1,20 @@
> > > +#!/bin/bash
> > > +#
> > > +# Oracle Linux DTrace.
> > > +# Copyright (c) 2022, 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.
> > > +#
> > > +
> > > +dtrace=$1
> > > +
> > > +$dtrace $dt_flags -Sen '
> > > +BEGIN
> > > +{
> > > +	self->foo = 42;
> > > +	trace(self->foo);
> > > +	exit(0);
> > > +}
> > > +' 2>&1 | awk '/foo/ && !/D type/ { print $8, $NF }'
> > > +
> > > +exit $?
> > > -- 
> > > 2.18.4
> > > 
> > > 
> > > _______________________________________________
> > > DTrace-devel mailing list
> > > DTrace-devel at oss.oracle.com
> > > https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel