[DTrace-devel] [PATCH 6/6] Add disassembler annotations for assoc arrays
Eugene Loh
eugene.loh at oracle.com
Thu Mar 10 01:24:43 UTC 2022
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?
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
More information about the DTrace-devel
mailing list