[DTrace-devel] [PATCH 6/6] Add disassembler annotations for assoc arrays
Kris Van Hees
kris.van.hees at oracle.com
Thu Mar 10 04:27:15 UTC 2022
On Wed, Mar 09, 2022 at 08:24:43PM -0500, Eugene Loh via DTrace-devel wrote:
> On 3/8/22 5:59 PM, Kris Van Hees wrote:
>
> > 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.
>
> Cool.
>
> But I saw a v2 of the patch. It does not use the proposed in[-8] trick
> (store imm vs store reg). Did the approach not pan out? Or you decided
> against it? Or oversight?
Oversight. Posted too soon (forgot to commit a file). Will be v3 soon.
> Also...
>
> >
> > > } 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.
> Right. But it does so by examining a sequence of instructions that is
> emitted by different code for load and store (at least for assoc). Anyhow,
> this is addressed in v2. So looks good.
> > 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.
> The same argument could perhaps be made about a value like 1 but I tried
> some fault injection and the test still happened to pass because... the code
> accidentally picked up the value 0! I agree that the specific numbering
> does not matter. I just think it's easy enough to reduce the chances of a
> spurious pass.
> > 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.
> Good point. Nice fix in v2.
> > 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
>
> _______________________________________________
> 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