[DTrace-devel] [PATCH 1/2 v2] Add annotations for dctx->X and mst->X members
Eugene Loh
eugene.loh at oracle.com
Mon Mar 28 21:37:29 UTC 2022
On 3/28/22 4:48 PM, Kris Van Hees wrote:
> On Mon, Mar 28, 2022 at 04:35:42PM -0400, Eugene Loh via DTrace-devel wrote:
>> Weren't there going to be tests?
> Argh, forgot to include them in the commit.
>
>> Looking just at the mst->X stuff, there are quite a few places where we
>> access mst->X. Looking at the dev branch, the only one of those cases that
>> this patch would pick up would be the mst->fault check after get_bvar().
>> That's not very much. Also not helpful since the get_bvar() is already
>> annotated and the stuff afterwards is some boilerplate that isn't made much
>> more helpful by adding mst->fault annotation.
>>
>> With the alloca() patch series, there may be more places to pick up mst->X
>> fairly easily. If that's of interest, then let's let the dust settle on
>> that patch series first. Improved disassembly can be done later.
>>
>> Granted, the narrow formula prescribed by this patch won't pick up all of
>> the new instances that the alloca() patch series will introduce, but just
>> loosening the formula up a little can improve things a lot. E.g. don't look
>> just in in[-1] and in[-2]. Rather, walk instructions backwards to look for
>> the instructions of interest. If something is a store, keep going.
>> Likewise if it's a load to an unrelated register. It's a little more
>> complicated, but not much and can sit in its own function.
>>
>> In short:
>> *) need tests
> Yes, I have the tests. Forgot to include them. My fault.
>
>> *) might as well do this after the alloca() patch series has stabilized
> I don't see why.
The reality is that the disassembler works by recognizing certain code
that we ourselves generate. Before the alloca() patch series, there is
only one specialized case we're talking about catching: the mst->fault
check after get_bvar(). With the patch series, there are many more
cases we might consider catching. Let's see what they end up being
rather than chase code sequences that might change.
> And in fact, it helps with reviewing the alloca patch,
This patch doesn't need tests, polishing up, or review to be used for
reviewing the alloca patch series. As you say below: "if we want to do
this, we should do it well." Meanwhile, this patch is a distraction
from getting the alloca() patch series through.
> and
> there is nothing here that needs waiting for.
>
>> *) then broaden the logic a little to pick up many more cases (I can do
>> this if you like)
> I'd like to do an improbement like this with a bit more planning, because if
> we want to do this, we should do it well, e.g. also track reg-to-reg moves so
> we can handle things like:
>
> lddw %r1, [%fp+DT_STK_DCTX] ! dctx
> lddw $r1, [%r1+DCTX_MST] ! dctx->mst
> << maybe some other code >>
> mov %r2, %r1
> lddw %r1, [%r1 + off] ! mst->X
> << maybe some other code >>
> lddw %21, [%r2 + off] ! mst->Y
>
> But requires a bit of design to handle disassembler state.
I don't see the point. This disassembly is only for code we generate;
I'm unaware that we generate any such code.
>> On 3/26/22 3:04 AM, Kris Van Hees via DTrace-devel wrote:
>>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>>> ---
>>> libdtrace/dt_dis.c | 128 +++++++++++++++++++++++++++++++++++----------
>>> 1 file changed, 100 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
>>> index 792cc99e..bc5eab54 100644
>>> --- a/libdtrace/dt_dis.c
>>> +++ b/libdtrace/dt_dis.c
>>> @@ -82,18 +82,20 @@ dt_dis_varname_off(const dtrace_difo_t *dp, uint_t off, uint_t scope, uint_t add
>>> }
>>> /*
>>> - * Check if we are loading from the gvar, lvar, or strtab BPF map.
>>> + * Check if we are accessing the gvars, lvars, or strtab BPF map, or the DTrace
>>> + * machine state.
>>> *
>>> - * If we are loading a gvar or lvar, we want to report the variable name.
>>> - * If we are loading a string constant, we want to report its value.
>>> + * If we are accessing the gvars or lvars map, we report the variable name.
>>> + * If we are accessing strtab map, we report its value.
>>> + * If we are accessing the DTrace machine state, we report the member name.
>>> *
>>> * For variables, the sequence of instructions we are looking for is:
>>> * insn code dst src offset imm
>>> * -2: ld dst %fp DT_STK_DCTX 00000000
>>> * -1: ld dst dst DCTX_*VARS 00000000
>>> - * 0: ld dst dst var_offset 00000000
>>> - * 0: st dst src var_offset 00000000
>>> - * 0: add dst 0 0 var_offset
>>> + * 0: ld dst dst offset 00000000 -or-
>>> + * 0: st dst src offset 00000000 -or-
>>> + * 0: add dst 0 0 offset
>>> * where instruction 0 is the current instruction, which may be one
>>> * of the three above cases. The three cases represent:
>>> * - load by value
>>> @@ -104,7 +106,17 @@ dt_dis_varname_off(const dtrace_difo_t *dp, uint_t off, uint_t scope, uint_t add
>>> * insn code dst src offset imm
>>> * -2: ld dst %fp DT_STK_DCTX 00000000
>>> * -1: ld dst dst DCTX_STRTAB 00000000
>>> - * 0: add dst 0 0 var_offset
>>> + * 0: add dst 0 0 offset
>>> + * where instruction 0 is the current instruction.
>>> + *
>>> + * For DTrace machine state members, the sequence of instructions we are
>>> + * looking for is:
>>> + * insn code dst src offset imm
>>> + * -2: ld dst %fp DT_STK_DCTX 00000000
>>> + * -1: ld dst dst DCTX_MST 00000000
>>> + * 0: ld dst dst offset 00000000 -or-
>>> + * 0: st dst src offset 00000000 -or-
>>> + * 0: add dst 0 0 offset
>>> * where instruction 0 is the current instruction.
>>> */
>>> static void
>>> @@ -113,7 +125,7 @@ dt_dis_refname(const dtrace_difo_t *dp, const struct bpf_insn *in, uint_t addr,
>>> {
>>> __u8 ldcode = BPF_LDX | BPF_MEM | BPF_DW;
>>> __u8 addcode = BPF_ALU64 | BPF_ADD | BPF_K;
>>> - int dst, scope = -1, var_offset = -1;
>>> + int dst, scope = -1, offset = -1;
>>> const char *str;
>>> /* make sure in[-2] and in[-1] exist */
>>> @@ -127,7 +139,7 @@ dt_dis_refname(const dtrace_difo_t *dp, const struct bpf_insn *in, uint_t addr,
>>> scope = DIFV_SCOPE_GLOBAL;
>>> else if (in[-1].off == DCTX_LVARS)
>>> scope = DIFV_SCOPE_LOCAL;
>>> - else if (in[-1].off != DCTX_STRTAB)
>>> + else if (in[-1].off != DCTX_STRTAB && in[-1].off != DCTX_MST)
>>> goto out;
>>> /* check preceding instructions */
>>> @@ -141,7 +153,7 @@ dt_dis_refname(const dtrace_difo_t *dp, const struct bpf_insn *in, uint_t addr,
>>> in[-1].imm != 0)
>>> goto out;
>>> - /* check the current instruction and read var_offset */
>>> + /* check the current instruction and read offset */
>>> if (in->dst_reg != dst)
>>> goto out;
>>> if (in[-1].off == DCTX_STRTAB) {
>>> @@ -156,21 +168,53 @@ dt_dis_refname(const dtrace_difo_t *dp, const struct bpf_insn *in, uint_t addr,
>>> } else if (BPF_CLASS(in->code) == BPF_LDX &&
>>> BPF_MODE(in->code) == BPF_MEM &&
>>> in->src_reg == dst && in->imm == 0)
>>> - var_offset = in->off;
>>> + offset = in->off;
>>> else if (BPF_CLASS(in->code) == BPF_STX &&
>>> BPF_MODE(in->code) == BPF_MEM &&
>>> in->src_reg != dst && in->imm == 0)
>>> - var_offset = in->off;
>>> + offset = in->off;
>>> else if (in->code == addcode && in->src_reg == 0 && in->off == 0)
>>> - var_offset = in->imm;
>>> + offset = in->imm;
>>> else
>>> goto out;
>>> /* print name */
>>> - str = dt_dis_varname_off(dp, var_offset, scope, addr);
>>> - if (str != NULL)
>>> - fprintf(fp, "%*s! %s%s", DT_DIS_PAD(n), "",
>>> - scope == DIFV_SCOPE_LOCAL ? "this->" : "", str);
>>> + if (in[-1].off == DCTX_MST) {
>>> + static const char *mst_members[sizeof(dt_mstate_t)] =
>>> + {
>>> + [0 ... sizeof(dt_mstate_t) - 1] = NULL,
>>> + [DMST_EPID] = "epid",
>>> + [DMST_PRID] = "prid",
>>> + [DMST_CLID] = "clid",
>>> + [DMST_TAG] = "tag",
>>> + [DMST_ERRNO] = "syscall_errno",
>>> + [DMST_FAULT] = "fault",
>>> + [DMST_TSTAMP] = "tstamp",
>>> + [DMST_REGS] = "regs",
>>> + [DMST_ARG(0)] = "arg0",
>>> + [DMST_ARG(1)] = "arg1",
>>> + [DMST_ARG(2)] = "arg2",
>>> + [DMST_ARG(3)] = "arg3",
>>> + [DMST_ARG(4)] = "arg4",
>>> + [DMST_ARG(5)] = "arg5",
>>> + [DMST_ARG(6)] = "arg6",
>>> + [DMST_ARG(7)] = "arg7",
>>> + [DMST_ARG(8)] = "arg8",
>>> + [DMST_ARG(9)] = "arg9",
>>> + };
>>> +
>>> + if (offset < 0 || offset >= sizeof(dt_mstate_t))
>>> + goto out;
>>> +
>>> + str = mst_members[offset];
>>> + if (str != NULL)
>>> + fprintf(fp, "%*s! mst->%s", DT_DIS_PAD(n), "", str);
>>> + } else {
>>> + str = dt_dis_varname_off(dp, offset, scope, addr);
>>> + if (str != NULL)
>>> + fprintf(fp, "%*s! %s%s", DT_DIS_PAD(n), "",
>>> + scope == DIFV_SCOPE_LOCAL ? "this->" : "", str);
>>> + }
>>> out:
>>> fprintf(fp, "\n");
>>> @@ -252,16 +296,43 @@ dt_dis_load(const dtrace_difo_t *dp, const char *name, uint_t addr,
>>> n = fprintf(fp, "%-4s %s, [%s%+d]", name, reg(in->dst_reg),
>>> reg(in->src_reg), in->off);
>>> - if (in->code == (BPF_LDX | BPF_MEM | BPF_DW) &&
>>> - in->src_reg == BPF_REG_FP &&
>>> - in->off <= DT_STK_SPILL(0) &&
>>> - in->off >= DT_STK_SPILL(DT_STK_NREGS) &&
>>> - in->dst_reg == -(in->off - DT_STK_SPILL_BASE) / DT_STK_SLOT_SZ) {
>>> - fprintf(fp, "%*s! restore %s\n", DT_DIS_PAD(n), "",
>>> - reg(in->dst_reg));
>>> - } else
>>> - dt_dis_refname(dp, in, addr, n, fp);
>>> + if (in->code == (BPF_LDX | BPF_MEM | BPF_DW)) {
>>> + if (in->src_reg == BPF_REG_FP && in->off <= DT_STK_SPILL(0) &&
>>> + in->off >= DT_STK_SPILL(DT_STK_NREGS) &&
>>> + in->dst_reg == -(in->off - DT_STK_SPILL_BASE) /
>>> + DT_STK_SLOT_SZ) {
>>> + fprintf(fp, "%*s! restore %s\n", DT_DIS_PAD(n), "",
>>> + reg(in->dst_reg));
>>> + return 0;
>>> + } else if (in->src_reg == in->dst_reg && addr > 1 &&
>>> + in[-1].code == (BPF_LDX | BPF_MEM | BPF_DW) &&
>>> + in[-1].src_reg == BPF_REG_FP &&
>>> + in[-1].dst_reg == in->src_reg &&
>>> + in[-1].off == DT_STK_DCTX &&
>>> + in->off >= 0 && in->off < DCTX_SIZE) {
>>> + static const char *dctx_member[DCTX_SIZE] = {
>>> + [0 ... DCTX_SIZE - 1] = NULL,
>>> + [DCTX_CTX] = "ctx",
>>> + [DCTX_ACT] = "act",
>>> + [DCTX_MST] = "mst",
>>> + [DCTX_BUF] = "buf",
>>> + [DCTX_MEM] = "mem",
>>> + [DCTX_STRTAB] = "strtab",
>>> + [DCTX_AGG] = "agg",
>>> + [DCTX_GVARS] = "gvars",
>>> + [DCTX_LVARS] = "lvars",
>>> + };
>>> + const char *s = dctx_member[in->off];
>>> +
>>> + if (s != NULL) {
>>> + fprintf(fp, "%*s! dctx->%s\n",
>>> + DT_DIS_PAD(n), "", s);
>>> + return 0;
>>> + }
>>> + }
>>> + }
>>> + dt_dis_refname(dp, in, addr, n, fp);
>>> return 0;
>>> }
>>> @@ -303,9 +374,10 @@ dt_dis_store(const dtrace_difo_t *dp, const char *name, uint_t addr,
>>> in->src_reg == -(in->off - DT_STK_SPILL_BASE) / DT_STK_SLOT_SZ) {
>>> fprintf(fp, "%*s! spill %s\n", DT_DIS_PAD(n), "",
>>> reg(in->src_reg));
>>> - } else
>>> - dt_dis_refname(dp, in, addr, n, fp);
>>> + return 0;
>>> + }
>>> + dt_dis_refname(dp, in, addr, n, fp);
>>> return 0;
>>> }
>> _______________________________________________
>> 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