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

[Kris Van Hees]

On Fri, Dec 17, 2021 at 02:27:37PM -0500, Eugene Loh via DTrace-devel wrote:
> On 12/17/21 1:16 AM, Kris Van Hees via DTrace-devel wrote:
> 
> > The disassembler generates the annotation for dt_get_tvar() based on the
> > wrong instruction.  It looks at the immediate value stored in the
> > (pc - 2) instruction whereas it should be looking at the one in pc - 3).
> Missing a '(' before "pc - 3"?  But I think the body of the commit message
> might be superfluous.  It's clear from the patch what's going on.

OK

> Meanwhile, the block comment preceding the code change appears to need
> updating.  How about:
>                  * We know that the previous two instruction exist and
> assigns
> s/two/three/
> s/instruction/instructions/
> s/assigns/assign/

Ah yes, thanks.

> Finally, if the patch fixes a bug, there should be a test that demonstrates
> the fix.  E.g., (stripped-down version for illustration; needs to be turned
> into .d .r .r.p for test suite; plus versions for gvar/lvar):

Why would we do this for gvar and lvar?  This is specific to tvar.  Yes, there
are not tests for annotations for those but then there are no tests for a lot
of the annotations.  I can add a separate patch to test this specific type of
annotation for gvar and lvar - it does not belong in this one.  It also seems
a bit random to add a patch for that but nothing else in the disassembler
annotation support.

So, I htink I will stick to a test for this bug and move on for now.  I will
add an issue on github to track adding tests for disaqssembler annotations in
general - to be done later.

> # dtrace -Sn 'BEGIN { self->foo = 1234; trace(self->foo); exit(0) }' |& awk
> '/foo$/ {print $NF}'
> self->foo
> self->foo
> # dtrace -Sn 'BEGIN { this->foo = 1234; trace(this->foo); exit(0) }' |& awk
> '/foo$/ {print $NF}'
> this->foo
> this->foo
> # dtrace -Sn 'BEGIN { foo = 1234; trace(foo); exit(0) }' |& awk '/foo$/
> {print $NF}'
> foo
> foo
> 
> > The generated sequence for calling dt_get_tvar is:
> > 
> >      (pc - 3)	mov %r1, <val>
> >      (pc - 2)	mov %r2, <val>
> >      (pc - 1)	mov %r3, <val>
> >      (pc)	call dt_get_tvar
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_dis.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> > index db8989a3..fc479274 100644
> > --- a/libdtrace/dt_dis.c
> > +++ b/libdtrace/dt_dis.c
> > @@ -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));
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel