[DTrace-devel] [PATCH] Add annotations for dctx->X and mst->X members
Kris Van Hees
kris.van.hees at oracle.com
Fri Mar 25 23:36:35 UTC 2022
On Fri, Mar 25, 2022 at 07:00:02PM -0400, Eugene Loh via DTrace-devel wrote:
> I haven't actually even finished reading this patch, but... I don't know.
>
> The disassembler figures out what's going on by reading the instructions and
> knowing under-the-hood specifics about the code generation. So, at the very
> least, there need to be some tests with this patch.
Sure, I will add tests for this. That is pretty easy.
> Further, it means that to review this patch, we need to know what branch to
> look at. This is a stand-alone patch. I'm guessing that this patch is
> intended to go on the alloca() patch series? I'm guessing.
I accidentally put the removal of some elements in the 2nd patch I posted,
which is why there are artifacts of the alloca code in this. I will post a
v2 with that cleanewd up (and some tests added), but even without that it
should be pretty easy to apply it to 'dev' if you wanted to test it.
And it should be pretty clear that the 'dev' branch is sufficient.
> Also, it's unclear to me what the objective is here. There might be some
> cases where we recognize certain constructs. Maybe we're okay not getting
> to all of them. That is, maybe we recognize mst->fault when it's accessed
> by dt_cg_check_fault(), but we do not recognize mst->prid or mst->errno when
> they are accessed by dt_cg_tramp_prologue_act(). Nor mst->epid or
> mst->tstamp or mst->fault or mst->clid when they are accessed by
> dt_cg_prologue(). But we do recognize mst->scalarizer in one case in
> dt_cg_check_bounds() but not in the other cases in the same function. In
> dt_cg_check_outscratch_bounds(), we recognize scratchtop but not
> scalarizer. In dt_cg_subr_alloca(), we recognize neither mst->scratch_top
> nor mst->scratch_next. The mst->X feature strikes me as very haphazard. Any
> correct annotation is a helpful annotation? Or does it have to be
> consistent enough that it feels reliable?
The trampoline ones are not ones we can capture without teaching the disassmbler
about tracking state of register values which is well beyond the scope of what
we are currently doing to provide annotations.
Sure, we do not capture all, but capturing common cases is certainly worth it.
Unless you are good at memorizing the offset values in dctx and mstate. I am
not.
> Again, I haven't finished reading the patch. I can take another look at it
> once I get a feeling for what is being sought here. The current patch seems
> to do a rather spotty job at, e.g., recognizing mst->X references. And
> there should be tests.
Simple: annotate dctx->X and mst->X accesses where possible. Not perfect,
but useful anyway.
Again, this patch is not meant to annotate every occurence of accesses to
dctx and mstate members. It is meant to annotate common cases to aid in
reading the disassembly listings.
>
> On 3/25/22 2:59 PM, Kris Van Hees via DTrace-devel wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/dt_dis.c | 132 +++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 104 insertions(+), 28 deletions(-)
> >
> > diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> > index 792cc99e..4a700ac6 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,56 @@ 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_SCRATCH_TOP] = "scratch_top",
> > + [DMST_SCRATCH_NEXT] = "scratch_next",
> > + [DMST_ERRNO] = "syscall_errno",
> > + [DMST_SCALARIZER] = "scalarizer",
> > + [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 +299,44 @@ 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_SCRATCHMEM] = "scratchmem",
> > + [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 +378,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