[DTrace-devel] [PATCH 6/6] Add disassembler annotations for assoc arrays
Kris Van Hees
kris.van.hees at oracle.com
Tue Mar 8 22:59:15 UTC 2022
On Tue, Mar 08, 2022 at 04:04:43PM -0500, Eugene Loh via DTrace-devel wrote:
> On 3/7/22 2:51 PM, Kris Van Hees via DTrace-devel wrote:
>
> > diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> > + } else if (strcmp(fn, "dt_get_assoc") == 0) {
> > + uint_t varid;
> > + dof_relodesc_t *rp = dp->dtdo_breltab;
> > + int cnt = dp->dtdo_brelen;
> > + const char *rname = NULL;
> > +
> > + /*
> > + * We know that the previous four instructions exist and
> > + * move the variable id to a register in the first instruction
> > + * of that seqeuence (because we wrote the code generator to
> > + * emit the instructions in this exact order.)
> > + */
> > + in -= 4;
> > + varid = in->imm + DIF_VAR_OTHER_UBASE;
> > +
> > + /*
> > + * We need to determine whether this is a associative TLS array
> > + * or a global array. We know that for a TLS assoc, a call to
> > + * dt_tlskey must be present 5 instructions earlier (actually
> > + * 9 instructions before the call to dt_get_assoc).
> > + */
> > + if (addr < 9 || in[-5].code != (BPF_JMP | BPF_CALL) ||
> > + in[-5].src_reg != BPF_PSEUDO_CALL)
> > + goto gvar_assoc;
> > +
> > + addr -= 9;
> > + for (; cnt; cnt--, rp++) {
> > + if (rp->dofr_offset > addr * sizeof(uint64_t))
> > + break;
> > + if (rp->dofr_offset == addr * sizeof(uint64_t))
> > + rname = dt_difo_getstr(dp, rp->dofr_name);
> > + }
> > +
> > + if (strcmp(rname, "dt_tlskey") != 0)
> > + goto gvar_assoc;
> > +
> > + snprintf(buf, len, "self->%s[]",
> > + dt_dis_varname_id(dp, varid, DIFV_SCOPE_THREAD, addr));
> > + return buf;
> > +
> > +gvar_assoc:
> > + snprintf(buf, len, "%s[]",
> > + dt_dis_varname_id(dp, varid, DIFV_SCOPE_GLOBAL, addr));
> > + return buf;
> > }
>
> If we really are going to rely on the fact that only we are generating
> dt_get_assoc() calls, it would seem that this reverse engineering could be a
> bunch easier. The 8th instruction before is either going to be a store reg
> or a store imm. That's the only signature we need. So how about:
Sure, we can do that. I was going on the notion that the main differentiating
factor was the use of dt_tlskey(), and this code was written before I added the
0 in case it is not a TLS variable. But, with proper comments added, I agree
that we can use the store from reg vs store imm distinction.
Thanks for catching that.
> } else if (strcmp(fn, "dt_get_assoc") == 0) {
> uint_t varid;
>
> /*
> * We know that the previous four instructions exist and
> * move the variable id to a register in the first instruction
> * of that seqeuence (because we wrote the code generator to
> * emit the instructions in this exact order.)
> */
> in -= 4;
> varid = in->imm + DIF_VAR_OTHER_UBASE;
>
> /*
> * Similarly, four instructions prior to that, there was either
> * a STORE or STORE_IMM operation, indicating TLS or global.
> */
> in -= 4;
> if (in->code == (BPF_STX | BPF_MEM | BPF_DW))
> snprintf(buf, len, "self->%s[]",
> dt_dis_varname_id(dp, varid, DIFV_SCOPE_THREAD, addr));
> else if (in->code == (BPF_ST | BPF_MEM | BPF_DW))
> snprintf(buf, len, "%s[]",
> dt_dis_varname_id(dp, varid, DIFV_SCOPE_GLOBAL, addr));
> else
> snprintf(buf, len, "???[]");
>
> return buf;
> }
>
> And one more thing...
>
> > diff --git a/test/unittest/disasm/tst.ann-gvar-assoc.r b/test/unittest/disasm/tst.ann-gvar-assoc.r
> > new file mode 100644
> > index 00000000..522ba6f6
> > --- /dev/null
> > +++ b/test/unittest/disasm/tst.ann-gvar-assoc.r
> > @@ -0,0 +1 @@
> > +85 0 1 0000 ffffffff call dt_get_assoc ! var[]
> > diff --git a/test/unittest/disasm/tst.ann-gvar-assoc.sh b/test/unittest/disasm/tst.ann-gvar-assoc.sh
> > new file mode 100755
> > index 00000000..c0fc4bf2
> > --- /dev/null
> > +++ b/test/unittest/disasm/tst.ann-gvar-assoc.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
> > +{
> > + var["foo"] = 42;
> > + exit(0);
> > +}
> > +' 2>&1 | awk '/ call dt_get_assoc/ { sub(/^[^:]+: /, ""); print; }'
> > +
> > +exit $?
> > diff --git a/test/unittest/disasm/tst.ann-tvar-assoc.r b/test/unittest/disasm/tst.ann-tvar-assoc.r
> > new file mode 100644
> > index 00000000..2ae89edb
> > --- /dev/null
> > +++ b/test/unittest/disasm/tst.ann-tvar-assoc.r
> > @@ -0,0 +1 @@
> > +85 0 1 0000 ffffffff call dt_get_assoc ! self->var[]
> > diff --git a/test/unittest/disasm/tst.ann-tvar-assoc.sh b/test/unittest/disasm/tst.ann-tvar-assoc.sh
> > new file mode 100755
> > index 00000000..e4690a28
> > --- /dev/null
> > +++ b/test/unittest/disasm/tst.ann-tvar-assoc.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["foo"] = 42;
> > + exit(0);
> > +}
> > +' 2>&1 | awk '/ call dt_get_assoc/ { sub(/^[^:]+: /, ""); print; }'
> > +
> > +exit $?
> There are two code paths -- dt_cg_store_var() and dt_cg_assoc_op() -- that
> emit dt_get_assoc() calls but the tests check only one of them: store.
> Also, they only examine varid==0 and it could be easy to stumble into such a
> value by luck. (That's what happened when I tried to break the tests!) So
> how about:
This test is not checking whether annotation are emitted for store and load
operations for associative arrays. It is a test for the disassembler
functionality to ensure that it correctly determines the annotations info for
TLS amd global associative arrays.
That also applies to whether varid is 0 or not. That is not relevant here.
If the correct annotation info is generated we know that the annotation
mechanisma works. Besides, the same argument can be made about any varid
value. A value like 1 is also easily stumbled upon. And again, we are not
testing for correct numbering of variables.
But if you are more comfortable with a test that applied to both code paths,
I can definitely add one or modify this one. But not as suggested because that
does not actually identify which one fails if you only get one line of output.
I'll post a v2 soon.
> diff --git a/test/unittest/disasm/tst.ann-gvar-assoc.r
> b/test/unittest/disasm/tst.ann-gvar-assoc.r
> @@ -1 +1,2 @@
> 85 0 1 0000 ffffffff call dt_get_assoc ! var[]
> +85 0 1 0000 ffffffff call dt_get_assoc ! var[]
> diff --git a/test/unittest/disasm/tst.ann-gvar-assoc.sh
> b/test/unittest/disasm/tst.ann-gvar-assoc.sh
> @@ -11,7 +11,9 @@ dtrace=$1
> $dtrace $dt_flags -Sen '
> BEGIN
> {
> + dummy = 1234;
> var["foo"] = 42;
> + trace(var["foo"]);
> exit(0);
> }
> ' 2>&1 | awk '/ call dt_get_assoc/ { sub(/^[^:]+: /, ""); print; }'
> diff --git a/test/unittest/disasm/tst.ann-tvar-assoc.r
> b/test/unittest/disasm/tst.ann-tvar-assoc.r
> @@ -1 +1,2 @@
> 85 0 1 0000 ffffffff call dt_get_assoc ! self->var[]
> +85 0 1 0000 ffffffff call dt_get_assoc ! self->var[]
> diff --git a/test/unittest/disasm/tst.ann-tvar-assoc.sh
> b/test/unittest/disasm/tst.ann-tvar-assoc.sh
> @@ -11,7 +11,9 @@ dtrace=$1
> $dtrace $dt_flags -Sen '
> BEGIN
> {
> + self->dummy = 1234;
> self->var["foo"] = 42;
> + trace(self->var["foo"]);
> exit(0);
> }
> ' 2>&1 | awk '/ call dt_get_assoc/ { sub(/^[^:]+: /, ""); print; }'
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list