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

[Kris Van Hees]

On Thu, Jan 27, 2022 at 01:37:41AM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> Also...
> 
> On 1/26/22 11:25 AM, Kris Van Hees via DTrace-devel wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_dis.c                          |  4 ++--
> >   test/unittest/variables/tvar/tst.dis-ann.r  |  1 +
> >   test/unittest/variables/tvar/tst.dis-ann.sh | 19 +++++++++++++++++++
> >   3 files changed, 22 insertions(+), 2 deletions(-)
> >   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/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> > index db8989a3..ebaa48bf 100644
> > --- a/libdtrace/dt_dis.c
> > +++ b/libdtrace/dt_dis.c
> > @@ -1,6 +1,6 @@
> >   /*
> >    * Oracle Linux DTrace.
> > - * Copyright (c) 2005, 2021, Oracle and/or its affiliates. All rights reserved.
> > + * Copyright (c) 2005, 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.
> >    */
> > @@ -312,7 +312,7 @@ dt_dis_bpf_args(const dtrace_difo_t *dp, const char *fn,
> >   		 * the variable id to %r1 (because we wrote the code generator
> >   		 * to emit these instructions in this exact order.)
> >   		 */
> > -		in -= 2;
> > +		in -= 3;
> >   		snprintf(buf, len, "self->%s",
> >   			 dt_dis_varname_id(dp, in->imm + DIF_VAR_OTHER_UBASE,
> >   					DIFV_SCOPE_THREAD, addr));
> 
> In earlier review, we talked about updating the associated comment block as
> part of this patch.  Specifically, in the line:
>         We know that the previous two instruction exist and assigns
> I could imagine:
>         s/two/three/
>         s/instruction/instructions/
>         s/assigns/assign/
> Are such changes reasonable to include in this patch?  At the very least, we
> have to talk about three, rather than two, previous instructions.

Huh, I made those changes.  Not sure where I managed to drop them.  Consider
them included.

> > 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..5194531b
> > --- /dev/null
> > +++ b/test/unittest/variables/tvar/tst.dis-ann.r
> > @@ -0,0 +1 @@
> > +85 0 1 0000 ffffffff    call dt_get_tvar              ! self->var
> > 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.