* Re: [DTrace-devel] [PATCH 1/2] Clarify how the usdt_prids key is stored on the BPF stack
@ 2025-03-19 14:47 Kris Van Hees
0 siblings, 0 replies; 3+ messages in thread
From: Kris Van Hees @ 2025-03-19 14:47 UTC (permalink / raw)
To: dtrace
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>
> ---
> 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));
> 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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [DTrace-devel] [PATCH 1/2] Clarify how the usdt_prids key is stored on the BPF stack
[not found] ` <Z9rXYvNQxZqODC3o@kvh-deb-bpf.us.oracle.com>
@ 2025-03-19 15:18 ` Kris Van Hees
2025-03-19 16:30 ` Eugene Loh
0 siblings, 1 reply; 3+ messages in thread
From: Kris Van Hees @ 2025-03-19 15:18 UTC (permalink / raw)
To: Kris Van Hees, dtrace; +Cc: eugene.loh@oracle.com, dtrace-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [DTrace-devel] [PATCH 1/2] Clarify how the usdt_prids key is stored on the BPF stack
2025-03-19 15:18 ` [DTrace-devel] " Kris Van Hees
@ 2025-03-19 16:30 ` Eugene Loh
0 siblings, 0 replies; 3+ messages in thread
From: Eugene Loh @ 2025-03-19 16:30 UTC (permalink / raw)
To: Kris Van Hees, dtrace; +Cc: dtrace-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-19 16:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-19 14:47 [DTrace-devel] [PATCH 1/2] Clarify how the usdt_prids key is stored on the BPF stack Kris Van Hees
-- strict thread matches above, loose matches on Subject: below --
2025-02-20 4:43 eugene.loh
[not found] ` <Z9rXYvNQxZqODC3o@kvh-deb-bpf.us.oracle.com>
2025-03-19 15:18 ` [DTrace-devel] " Kris Van Hees
2025-03-19 16:30 ` Eugene Loh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox