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

[Eugene Loh]

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.
  */

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.

> +		    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 &&