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

[Eugene Loh <eugene.loh@oracle.com>]

On 3/19/25 11:18, Kris Van Hees wrote:

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

Thanks.  Might this correction already be in the 2/2 patch?  (Not that 
that's the right place for it, but...)

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