Re: [DTrace-devel] [PATCH 1/2] Clarify how the usdt_prids key is stored on the BPF stack

[Kris Van Hees <kris.van.hees@oracle.com>]

On Wed, Mar 19, 2025 at 10:40:34AM -0400, Kris Van Hees via DTrace-devel wrote:
> On Wed, Feb 19, 2025 at 11:43:49PM -0500, eugene.loh@oracle.com wrote:
> > 
> > While one can access the BPF stack relative to %r9, the whole
> > point of DT_TRAMP_SP_SLOT(0) is to make trampoline code more
> > readable.  So use it.
> > 
> > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> 
> Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

Still applies but see below...

> > ---
> >  libdtrace/dt_prov_uprobe.c | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> > index 5d9f74244..f1323cc31 100644
> > --- a/libdtrace/dt_prov_uprobe.c
> > +++ b/libdtrace/dt_prov_uprobe.c
> > @@ -1015,22 +1015,15 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> >  	emit(dlp,  BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
> >  
> >  	/*
> > -	 * Look up in the BPF 'usdt_prids' map.  Space for the look-up key
> > -	 * will be used on the BPF stack:
> > -	 *
> > -	 *     offset                                       value
> > -	 *
> > -	 *     -sizeof(usdt_prids_map_key_t)                pid (in %r0)
> > -	 *
> > -	 *     -sizeof(usdt_prids_map_key_t) + sizeof(pid_t)
> > -	 *     ==
> > -	 *     -sizeof(dtrace_id_t)                         underlying-probe prid
> > +	 * Look up in the BPF 'usdt_prids' map.  The key should fit into
> > +	 * trampoline stack slot 0.
> >  	 */
> > -	emit(dlp,  BPF_STORE(BPF_W, BPF_REG_9, (int)(-sizeof(usdt_prids_map_key_t)), BPF_REG_0));
> > -	emit(dlp,  BPF_STORE_IMM(BPF_W, BPF_REG_9, (int)(-sizeof(dtrace_id_t)), uprp->desc->id));
> > +	assert(sizeof(usdt_prids_map_key_t) <= DT_STK_SLOT_SZ);
> > +	emit(dlp,  BPF_STORE(BPF_W, BPF_REG_FP, DT_TRAMP_SP_SLOT(0), BPF_REG_0));
> > +	emit(dlp,  BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_TRAMP_SP_SLOT(0) + sizeof(pid_t), uprp->desc->id));

I get a compiler warning here:

libdtrace/dt_prov_uprobe.c: In function â?~trampolineâ?T:
include/bpf_asm.h:119:24: warning: overflow in conversion from â?~long unsigned intâ?T to â?~short intâ?T changes value from â?~18446744073709551524â?T to â?~-92â?T [-Woverflo]
  119 |                 .off = (ofs),                                           \
      |                        ^
libdtrace/dt_as.h:42:69: note: in definition of macro â?~emitleâ?T
   42 |                  dt_irnode_t *dip = dt_cg_node_alloc((lbl), (instr)); \
      |                                                              ^~~~~

libdtrace/dt_prov_uprobe.c:1013:9: note: in expansion of macro â?~emitâ?T
 1013 |         emit(dlp,  BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_TRAMP_SP_SLOT(0) + sizeof(pid_t), uprp->desc->id));
      |         ^~~~
libdtrace/dt_prov_uprobe.c:1013:20: note: in expansion of macro â?~BPF_STORE_IMMâ?T
 1013 |         emit(dlp,  BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_TRAMP_SP_SLOT(0) + sizeof(pid_t), uprp->desc->id));

You need a (int) cast for sizeof(pid_t) similar to the casts that were in
the code before.  I'll add it in as I merge.

> >  	dt_cg_xsetx(dlp, usdt_prids, DT_LBL_NONE, BPF_REG_1, usdt_prids->di_id);
> > -	emit(dlp,  BPF_MOV_REG(BPF_REG_2, BPF_REG_9));
> > -	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, (int)(-sizeof(usdt_prids_map_key_t))));
> > +	emit(dlp,  BPF_MOV_REG(BPF_REG_2, BPF_REG_FP));
> > +	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DT_TRAMP_SP_SLOT(0)));
> >  	emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_map_lookup_elem));
> >  	emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_exit));
> >  
> > -- 
> > 2.43.5
> > 
> > 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel