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

[Eugene Loh]

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.

> 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)"?