[DTrace-devel] [PATCH] Add annotations for dctx->X and mst->X members
Eugene Loh
eugene.loh at oracle.com
Fri Mar 25 23:00:02 UTC 2022
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.
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.
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?
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.
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;
> }
>
More information about the DTrace-devel
mailing list