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

[Eugene Loh]

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.

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.

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.