[DTrace-devel] [PATCH 16/16] dis: add support for variables used in trampoline code

[Kris Van Hees]

On Thu, Jan 18, 2024 at 11:43:41PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> with comments...
> 
> On 1/12/24 19:24, Kris Van Hees via DTrace-devel wrote:
> > diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> > @@ -90,6 +90,11 @@ dt_dis_varname_off(const dtrace_difo_t *dp, uint_t off, uint_t scope, uint_t add
> >    *
> >    * For variables, the sequence of instructions we are looking for is:
> >    *         insn   code  dst  src    offset        imm
> > + *          -1:    ld   dst  %r9  DCTX_*VARS   00000000
> > + *           0:    ld   xxx  dst  var_offset   00000000
> > + *           0:    st   dst  src  var_offset   00000000
> > + *           0:    add  dst    0     0         var_offset
> > + * -or-
> >    *          -2:    ld   dst  %fp  DT_STK_DCTX  00000000
> >    *          -1:    ld   dst  dst  DCTX_*VARS   00000000
> >    *           0:    ld   xxx  dst  var_offset   00000000
> > @@ -103,6 +108,9 @@ dt_dis_varname_off(const dtrace_difo_t *dp, uint_t off, uint_t scope, uint_t add
> >    *
> >    * For string constants, the sequence of instructions we are looking for is:
> >    *         insn   code  dst  src    offset        imm
> > + *          -1:    ld   dst  %r9  DCTX_STRTAB  00000000
> > + *           0:    add  dst    0     0         var_offset
> > + * -or-
> >    *          -2:    ld   dst  %fp  DT_STK_DCTX  00000000
> >    *          -1:    ld   dst  dst  DCTX_STRTAB  00000000
> >    *           0:    add  dst    0     0         var_offset
> 
> Judging comments is a subjective matter, but the comments here IMHO can be
> "factored" apart.  Specifically, this whole disasm stuff is tied intimately
> to what we do in cg, and so specific references to what we're doing in cg
> strikes me not only as fair but even as helpful.  And referring to
> dt_cg_access_dctx() explicitly helps (me) understand what's going on and
> separates out something that is common to both vars and strings.  Listing
> instructions -2 and -1 twice (once for vars and once for strings) in the
> comments is not only redundant but also does not follow what happens in the
> actual code.  Also, since the in-code comments refer to trampolines, so it
> makes sense for the comment block to follow suit.
> 
> In all, here is my suggestion for the comment block:
> 
> /*
>  * Check if we are loading from the gvar, lvar, or strtab BPF map.
>  *
>  * 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.
>  *
>  * For variables, the current instruction (instruction 0) is one of:
>  *         insn   code  dst  src    offset        imm
>  *           0:    ld   xxx  dst  var_offset   00000000
>  *           0:    st   dst  src  var_offset   00000000
>  *           0:    add  dst    0     0         var_offset
>  * representing:
>  *   - load by value
>  *   - store by value
>  *   - access by reference
>  *
>  * For string constants, the current instruction is:
>  *           0:    add  dst    0     0         var_offset
>  *
>  * In either case, the preceding instructions are generated by
>  * dt_cg_access_dctx() and are:
>  *     if not trampoline:
>  *         insn   code  dst  src    offset        imm
>  *          -2:    ld   dst  %fp  DT_STK_DCTX  00000000
>  *          -1:    ld   dst  dst  DCTX_xxx     00000000
>  *     if trampoline:
>  *          -1:    ld   dst  %r9  DCTX_xxx     00000000
>  * where DCTX_xxx is DCTX_LVARS, DCTX_GVARS, or DCTX_STRTAB.
>  *
>  * In all cases, if a relocation symbol name is provided, report that.
>  */

Sure.  Thanks.

> Also...
> 
> > @@ -125,13 +133,28 @@ dt_dis_refname(const dtrace_difo_t *dp, const struct bpf_insn *in, uint_t addr,
> >   		goto out;
> >   	}
> > -	/* make sure in[-2] and in[-1] exist */
> > -	if (addr < 2)
> > +	/* make sure in[-1] exists and is a ld instruction */
> > +	if (addr < 1 || in[-1].code != ldcode || in[-1].imm != 0)
> >   		goto out;
> > -	/* get dst and scope */
> > +	/* get dst register */
> >   	dst = in[-1].dst_reg;
> > +	/* trampoline code uses %r9, regular code needs to load dctx */
> > +	if (in[-1].src_reg != BPF_REG_9) {
> > +		/* make sure in[-2] exists and is a ld instruction */
> > +		if (addr < 2 || in[-2].code != ldcode || in[-2].imm != 0)
> > +			goto out;
> > +
> > +		/* check preceding instructions */
> > +		if (in[-2].dst_reg != dst ||
> > +		    in[-2].src_reg != BPF_REG_FP ||
> > +		    in[-2].off != DT_STK_DCTX ||
> > +		    in[-1].code != ldcode ||
> 
> We already checked that in[-1].code==ldcode.

Ah yes, thank you.

> > +		    in[-1].src_reg != dst)
> > +			goto out;
> > +	}
> > +
> >   	if (in[-1].off == DCTX_GVARS)
> >   		scope = DIFV_SCOPE_GLOBAL;
> >   	else if (in[-1].off == DCTX_LVARS)
> > @@ -139,17 +162,6 @@ dt_dis_refname(const dtrace_difo_t *dp, const struct bpf_insn *in, uint_t addr,
> >   	else if (in[-1].off != DCTX_STRTAB)
> >   		goto out;
> > -	/* check preceding instructions */
> > -	if (in[-2].code != ldcode ||
> > -	    in[-2].dst_reg != dst ||
> > -	    in[-2].src_reg != BPF_REG_FP ||
> > -	    in[-2].off != DT_STK_DCTX ||
> > -	    in[-2].imm != 0 ||
> > -	    in[-1].code != ldcode ||
> > -	    in[-1].src_reg != dst ||
> > -	    in[-1].imm != 0)
> > -		goto out;
> > -
> >   	/* check the current instruction and read var_offset */
> >   	if (in[-1].off == DCTX_STRTAB) {
> >   		if (in->dst_reg == dst &&
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel