[DTrace-devel] [PATCH v2] Fix annotation for dt_get_tvar()

[Kris Van Hees]

On Thu, Jan 27, 2022 at 12:41:30PM -0500, Eugene Loh via DTrace-devel wrote:
> On 1/27/22 9:03 AM, Kris Van Hees wrote:
> 
> > On Thu, Jan 27, 2022 at 01:37:41AM -0500, Eugene Loh via DTrace-devel wrote:
> > > On 1/26/22 11:25 AM, Kris Van Hees via DTrace-devel wrote:
> > > > 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..5afe3978
> > > > --- /dev/null
> > > > +++ b/test/unittest/variables/tvar/tst.dis-ann.sh
> > > > @@ -0,0 +1,19 @@
> > > > +#!/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->var = 42;;
> > > > +	exit(0);
> > > > +}
> > > > +' 2>&1 | awk '/ call dt_[gs]et_tvar/ { sub(/^[^:]+: /, ""); print; }'
> > > > +
> > > > +exit $?
> > > Why the double semicolon for the 42?  And why dt_[gs]et_tvar rather than
> > > just dt_get_tvar?  Also, how about adding a read -- e.g., adding
> > > "trace(self->var)"?
> > ;; is a type
> > [gs] is because my brain forgot that I got rid of set_tvar long ago.
> > No need to print the output because we never execute the script.
> I was suggesting trace(self->var) not to see the contents of self->var but
> to see the disassembly.  The test checks disassembly for *setting* a tvar. 
> I'm suggesting the test could also check disassembly for *getting* a tvar. 
> One can say that we know they're the same thing, but the broader test
> coverage makes sense to me.

The test is about the annotation for the get_tvar call.  We do not get any
broader coverage when also retrieving the TLS var in a trace() action.  We
already have tests that exercise storing and retrieving TLS variables.

> Specifically, we ought to have similar tests for self-> and global
> variables.  (I can post a patch if you like.)  We might as well reduce
> gratuitous differences between the tests.  For self-> and global variables,
> there are indeed differences in the mechanisms for reading and writing the
> variables.  So, adding a "trace()" does make sense for those cases.  No
> sense in eliminating the trace() check for tvar just because we happen to
> know that currently the implementation does not do anything different.

Again, there is a difference bwtween exercising the annotation mechanism and
the mechanism for storing and retrievig variables.  Yes, as I mentioned in my
response to the v1 of the patch, we should at some point add tests for global
and local variable annotations as well.  Feel free to create those and post a
patch, or I will get to it soonish.

> Indeed, instead of /call dt[gs]et_tvar/, how about naming the variable "foo"
> and using /foo/ instead?  That, also, will reduce gratuitous differences
> among the tests.

I am testing that we get the correct annotation, and that we get it on the
correct instruction.

	Kris