hm.. I thought we discussed recording the number of bits to shift by, no? It's not too big of a deal, we can afford 4 bits (instead of 2 that would be enough for bit shift), but any specific reason you prefer multiplication here?

Using multiplication is more straightforward since the format of a SIB address looks like `8@(%rdx,%rax,8)`. We can directly store the 8 as a scale instead of translating it to the number of bits to shift by.

"defaulting to 1" is very confusing, why? (and please use spaces after comma)

Yes, it's misleading to put it down here. The default value of scale is 1 since the scale can be omitted when the scale is 1. I remove it in the next version.

At 2025-08-23 06:43:06, "Andrii Nakryiko" andrii.nakryiko@gmail.com wrote:

On Fri, Aug 22, 2025 at 8:16 AM Jiawei Zhao phoenix500526@163.com wrote:

...

On x86-64, USDT arguments can be specified using Scale-Index-Base (SIB) addressing, e.g. "1@-96(%rbp,%rax,8)". The current USDT implementation in libbpf cannot parse this format, causing `bpf_program__attach_usdt()` to fail with -ENOENT (unrecognized register).

This patch fixes this by implementing the necessary changes:

• add correct handling for SIB-addressed arguments in `bpf_usdt_arg`.
• add adaptive support to `__bpf_usdt_arg_type` and `__bpf_usdt_arg_spec` to represent SIB addressing parameters.

Signed-off-by: Jiawei Zhao phoenix500526@163.com

tools/lib/bpf/usdt.bpf.h | 47 ++++++++++++++++++++++++++++++-- tools/lib/bpf/usdt.c | 58 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 98 insertions(+), 7 deletions(-)

diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h index 2a7865c8e3fe..263168d57286 100644 --- a/tools/lib/bpf/usdt.bpf.h +++ b/tools/lib/bpf/usdt.bpf.h @@ -4,6 +4,7 @@ #define __USDT_BPF_H__

#include <linux/errno.h> +#include <asm/byteorder.h> #include "bpf_helpers.h" #include "bpf_tracing.h"

@@ -34,13 +35,34 @@ enum __bpf_usdt_arg_type { BPF_USDT_ARG_CONST, BPF_USDT_ARG_REG, BPF_USDT_ARG_REG_DEREF,

•   BPF_USDT_ARG_SIB,
  

};

+/*

• • This struct layout is designed specifically to be backwards/forward
• • compatible between libbpf versions for ARG_CONST, ARG_REG, and
• • ARG_REG_DEREF modes. ARG_SIB requires libbpf v1.7+.
• */

struct __bpf_usdt_arg_spec { /* u64 scalar interpreted depending on arg_type, see below */ __u64 val_off; /* arg location case, see bpf_usdt_arg() for details */

•   enum __bpf_usdt_arg_type arg_type;
  

•   enum __bpf_usdt_arg_type arg_type: 8;
  

this needs to be inside the #if/#elif/#else, I believe. When it previously was a 4 byte field, BPF_USDT_ARG_REG = 1 would be `0x01, 0x00, 0x00, 0x00` in memory on little endian and `0x00, 0x00, 0x00, 0x01` on big endian. So you need to reorder it such that on big endian it's the last field.

pw-bot: cr

...

+#if defined(__LITTLE_ENDIAN_BITFIELD)

I'm not sure whether compiler itself defines __LITTLE_ENDIAN_BITFIELD. Throughout libbpf we use #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__, let's do that here as well?

...

•   /* index register offset within struct pt_regs (high 12 bits) */
  

•   __u16   idx_reg_off: 12,
  

•   /* scale factor for index register (1, 2, 4, or 8) (low 4 bits) */
  

•           scale: 4;
  

nit: don't do comma-separated bitfields. compiler will combine them as necessary, even if they are declared as separate fields. so just:

#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ enum __bpf_usdt_arg_type arg_type: 8; __u16 idx_reg_off: 12; __u16 idx_reg_shift: 4; __u8 __reserved: 8; #else __u8 __reserved: 8; __u16 idx_reg_off: 12; __u16 idx_reg_shift: 4; enum __bpf_usdt_arg_type arg_type: 8; #endif

Note that we don't need to change order of idx_reg_off and idx_reg_shift, as they are new additions (and they don't have to be consistent between big and little endian)

...

+#elif defined(__BIG_ENDIAN_BITFIELD)

•   /* scale factor for index register (1, 2, 4, or 8) (high 4 bits) */
  

•   __u16   scale: 4,
  

•   /* index register offset within struct pt_regs (low 12 bits) */
  

•           idx_reg_off: 12;
  

+#else +#error "Please fix <asm/byteorder.h>" +#endif

let's drop the fix suggestion, isn't asm/byteorder.h kernel-specific header?.. I'm fine assuming only big or little endian system

...

•   /* reserved for future use, keeps reg_off offset stable */
  

•   __u8 reserved;
    /* offset of referenced register within struct pt_regs */
    short reg_off;
    /* whether arg should be interpreted as signed value */
  

@@ -149,7 +171,7 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res) { struct __bpf_usdt_spec *spec; struct __bpf_usdt_arg_spec *arg_spec;

•   unsigned long val;
  

•   unsigned long val, idx;
    int err, spec_id;
  
    *res = 0;
  

@@ -202,6 +224,27 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res) return err; #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ val >>= arg_spec->arg_bitshift; +#endif

•           break;
  

•   case BPF_USDT_ARG_SIB:
  

•           /* Arg is in memory addressed by SIB (Scale-Index-Base) mode
  

•            * (e.g., "-1@-96(%rbp,%rax,8)" in USDT arg spec). We first
  

•            * fetch the base register contents and the index register
  

•            * contents from pt_regs. Then we calculate the final address
  

•            * as base + (index * scale) + offset, and do a user-space
  

•            * probe read to fetch the argument value.
  

•            */
  

•           err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
  

•           if (err)
  

•                   return err;
  

•           err = bpf_probe_read_kernel(&idx, sizeof(idx), (void *)ctx + arg_spec->idx_reg_off);
  

•           if (err)
  

•                   return err;
  

•           err = bpf_probe_read_user(&val, sizeof(val), (void *)(val + (idx * arg_spec->scale) + arg_spec->val_off));
  

hm.. I thought we discussed recording the number of bits to shift by, no? It's not too big of a deal, we can afford 4 bits (instead of 2 that would be enough for bit shift), but any specific reason you prefer multiplication here?

...

•           if (err)
  

•                   return err;
  

+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__

•           val >>= arg_spec->arg_bitshift;
  

#endif break; default:

[...]

...

•   if (sscanf(arg_str, " %d @ %ld ( %%%15[^)] ) %n", arg_sz, &off, reg_name, &len) == 3) {
  

•           reg_off = calc_pt_regs_off(reg_name);
  

•           if (reg_off < 0)
  

•                   return reg_off;
  

•           arg->reg_off = reg_off;
  

•           idx_reg_off = calc_pt_regs_off(idx_reg_name);
  

•           if (idx_reg_off < 0)
  

•                   return idx_reg_off;
  

•           /* validate scale factor and set fields directly */
  

•           if (scale != 1 && scale != 2 && scale != 4 && scale != 8) {
  

•                   pr_warn("usdt: invalid SIB scale %d, expected 1,2,4,8; defaulting to 1\n", scale);
  

"defaulting to 1" is very confusing, why? (and please use spaces after comma)

...

•                   return -EINVAL;
  

•           }
  

•           arg->idx_reg_off = idx_reg_off;
  

•           arg->scale = scale;
  

•   } else if (sscanf(arg_str, " %d @ %ld ( %%%15[^)] ) %n",
  

•                           arg_sz, &off, reg_name, &len) == 3) {
            /* Memory dereference case, e.g., -4@-20(%rbp) */
            arg->arg_type = USDT_ARG_REG_DEREF;
            arg->val_off = off;
  

@@ -1306,6 +1353,7 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec } else if (sscanf(arg_str, " %d @ %%%15s %n", arg_sz, reg_name, &len) == 2) { /* Register read case, e.g., -4@%eax */ arg->arg_type = USDT_ARG_REG;

•           /* register read has no memory offset */
            arg->val_off = 0;
  
            reg_off = calc_pt_regs_off(reg_name);
  

-- 2.43.0

On Fri, Aug 22, 2025 at 8:16 AM Jiawei Zhao phoenix500526@163.com wrote:

When using GCC on x86-64 to compile an usdt prog with -O1 or higher optimization, the compiler will generate SIB addressing mode for global array and PC-relative addressing mode for global variable, e.g. "1@-96(%rbp,%rax,8)" and "-1@4+t1(%rip)".

In this patch:

• enrich subtest_basic_usdt test case to cover SIB addressing usdt argument spec handling logic

Signed-off-by: Jiawei Zhao phoenix500526@163.com

tools/testing/selftests/bpf/prog_tests/usdt.c | 44 ++++++++++++++++++- tools/testing/selftests/bpf/progs/test_usdt.c | 30 +++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c index 9057e983cc54..c04b416aa4a8 100644 --- a/tools/testing/selftests/bpf/prog_tests/usdt.c +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c @@ -25,6 +25,10 @@ unsigned short test_usdt0_semaphore SEC(".probes"); unsigned short test_usdt3_semaphore SEC(".probes"); unsigned short test_usdt12_semaphore SEC(".probes");

+#if ((defined(__x86_64__) || defined(__i386__)) && defined(__GNUC__) && !defined(__clang__))

does clang define __GNUC__ as well? otherwise why !define(__clang__) ?

+unsigned short test_usdt_sib_semaphore SEC(".probes"); +#endif

static void __always_inline trigger_func(int x) { long y = 42;

@@ -40,12 +44,29 @@ static void __always_inline trigger_func(int x) { } }

+#if ((defined(__x86_64__) || defined(__i386__)) && defined(__GNUC__) && !defined(__clang__)) +static __attribute__((optimize("O1"))) void trigger_sib_spec(void)

you use assembly directly, so optimize() should be irrelevant, no?

So we can make this non-GCC specific, right?

+{

•   /* Base address + offset + (index * scale) */
  

•   /* Force SIB addressing with inline assembly */
  

•   asm volatile(
  

•           "# probe point with memory access\n"
  

•           STAP_PROBE_ASM(test, usdt_sib, -2@(%%rdx,%%rax,2))
  

is it guaranteed that nums address will end up in rdx and a in rax?...

I'd feel more comfortable if you explicitly set up rdx and rax in assembly, then add USDT with STAP_PROBE_ASM. That should be possible with embedded assembly, no?

•           "# end probe point"
  

why these unnecessary comments embedded in the assembly?...

•           :
  

•           : "d"(nums), "a"(0)
  

•           : "memory"
  

•   );
  

+}

[...]

+int usdt_sib_called; +u64 usdt_sib_cookie; +int usdt_sib_arg_cnt; +int usdt_sib_arg_ret; +u64 usdt_sib_arg; +int usdt_sib_arg_size;

+// Note: usdt_sib is only tested on x86-related architectures, so it requires +// manual attach since auto-attach will panic tests under other architectures

don't use c++ style comments

+SEC("usdt") +int usdt_sib(struct pt_regs *ctx) +{

•   long tmp;
  

•   if (my_pid != (bpf_get_current_pid_tgid() >> 32))
  

•           return 0;
  

•   __sync_fetch_and_add(&usdt_sib_called, 1);
  

•   usdt_sib_cookie = bpf_usdt_cookie(ctx);
  

•   usdt_sib_arg_cnt = bpf_usdt_arg_cnt(ctx);
  

•   usdt_sib_arg_ret = bpf_usdt_arg(ctx, 0, &tmp);
  

•   usdt_sib_arg = (short)tmp;
  

•   usdt_sib_arg_size = bpf_usdt_arg_size(ctx, 0);
  

•   return 0;
  

+}

char _license[] SEC("license") = "GPL";

2.43.0

...

•           STAP_PROBE_ASM(test, usdt_sib, -2@(%%rdx,%%rax,2))
  

I think such hard-coding, for example -2@(%%rdx,%%rax,2), is not very maintainable. Essentially, STAP_PROBE_ASM just performs simple text substitution. But in SIB addressing, both scale and size depend on the actual parameter type.

For instance, in -2@(%%rdx,%%rax,2), the -2 and 2 come from the fact that nums is an array of type short: the size of a short is 2, so the scale is 2; and since short is a signed type, the size is -2.

STAP_PROBE_ASM is expanded at the preprocessing stage, while at that stage we cannot compute the actual size of nums. If the type of nums changes in the future, it could lead to errors.

At 2025-08-23 06:59:49, "Andrii Nakryiko" andrii.nakryiko@gmail.com wrote:

On Fri, Aug 22, 2025 at 8:16 AM Jiawei Zhao phoenix500526@163.com wrote:

...

When using GCC on x86-64 to compile an usdt prog with -O1 or higher optimization, the compiler will generate SIB addressing mode for global array and PC-relative addressing mode for global variable, e.g. "1@-96(%rbp,%rax,8)" and "-1@4+t1(%rip)".

In this patch:

• enrich subtest_basic_usdt test case to cover SIB addressing usdt argument spec handling logic

Signed-off-by: Jiawei Zhao phoenix500526@163.com

tools/testing/selftests/bpf/prog_tests/usdt.c | 44 ++++++++++++++++++- tools/testing/selftests/bpf/progs/test_usdt.c | 30 +++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c index 9057e983cc54..c04b416aa4a8 100644 --- a/tools/testing/selftests/bpf/prog_tests/usdt.c +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c @@ -25,6 +25,10 @@ unsigned short test_usdt0_semaphore SEC(".probes"); unsigned short test_usdt3_semaphore SEC(".probes"); unsigned short test_usdt12_semaphore SEC(".probes");

+#if ((defined(__x86_64__) || defined(__i386__)) && defined(__GNUC__) && !defined(__clang__))

does clang define __GNUC__ as well? otherwise why !define(__clang__) ?

...

+unsigned short test_usdt_sib_semaphore SEC(".probes"); +#endif

static void __always_inline trigger_func(int x) { long y = 42;

@@ -40,12 +44,29 @@ static void __always_inline trigger_func(int x) { } }

+#if ((defined(__x86_64__) || defined(__i386__)) && defined(__GNUC__) && !defined(__clang__)) +static __attribute__((optimize("O1"))) void trigger_sib_spec(void)

you use assembly directly, so optimize() should be irrelevant, no?

So we can make this non-GCC specific, right?

...

+{

•   /* Base address + offset + (index * scale) */
  

•   /* Force SIB addressing with inline assembly */
  

•   asm volatile(
  

•           "# probe point with memory access\n"
  

•           STAP_PROBE_ASM(test, usdt_sib, -2@(%%rdx,%%rax,2))
  

is it guaranteed that nums address will end up in rdx and a in rax?...

I'd feel more comfortable if you explicitly set up rdx and rax in assembly, then add USDT with STAP_PROBE_ASM. That should be possible with embedded assembly, no?

...

•           "# end probe point"
  

why these unnecessary comments embedded in the assembly?...

...

•           :
  

•           : "d"(nums), "a"(0)
  

•           : "memory"
  

•   );
  

+}

[...]

...

+int usdt_sib_called; +u64 usdt_sib_cookie; +int usdt_sib_arg_cnt; +int usdt_sib_arg_ret; +u64 usdt_sib_arg; +int usdt_sib_arg_size;

+// Note: usdt_sib is only tested on x86-related architectures, so it requires +// manual attach since auto-attach will panic tests under other architectures

don't use c++ style comments

...

+SEC("usdt") +int usdt_sib(struct pt_regs *ctx) +{

•   long tmp;
  

•   if (my_pid != (bpf_get_current_pid_tgid() >> 32))
  

•           return 0;
  

•   __sync_fetch_and_add(&usdt_sib_called, 1);
  

•   usdt_sib_cookie = bpf_usdt_cookie(ctx);
  

•   usdt_sib_arg_cnt = bpf_usdt_arg_cnt(ctx);
  

•   usdt_sib_arg_ret = bpf_usdt_arg(ctx, 0, &tmp);
  

•   usdt_sib_arg = (short)tmp;
  

•   usdt_sib_arg_size = bpf_usdt_arg_size(ctx, 0);
  

•   return 0;
  

+}

char _license[] SEC("license") = "GPL";

2.43.0