| Message ID | 20240428112559.10518-1-jose.marchesi@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cf9bea94f6b2934d409511c05337010b137316a3 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: fix bpf_ksym_exists in GCC | expand |
On 4/28/24 4:25 AM, Jose E. Marchesi wrote: > The macro bpf_ksym_exists is defined in bpf_helpers.h as: > > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > !!sym; \ > }) > > The purpose of the macro is to determine whether a given symbol has > been defined, given the address of the object associated with the > symbol. It also has a compile-time check to make sure the object > whose address is passed to the macro has been declared as weak, which > makes the check on `sym' meaningful. > > As it happens, the check for weak doesn't work in GCC in all cases, > because __builtin_constant_p not always folds at parse time when > optimizing. This is because optimizations that happen later in the > compilation process, like inlining, may make a previously non-constant > expression a constant. This results in errors like the following when > building the selftests with GCC: > > bpf_helpers.h:190:24: error: expression in static assertion is not constant > 190 | _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Fortunately recent versions of GCC support a __builtin_has_attribute > that can be used to directly check for the __weak__ attribute. This > patch changes bpf_helpers.h to use that builtin when building with a > recent enough GCC, and to omit the check if GCC is too old to support > the builtin. > > The macro used for GCC becomes: > > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ > !!sym; \ > }) > > Note that since bpf_ksym_exists is designed to get the address of the > object associated with symbol SYM, we pass *sym to > __builtin_has_attribute instead of sym. When an expression is passed > to __builtin_has_attribute then it is the type of the passed > expression that is checked for the specified attribute. The > expression itself is not evaluated. This accommodates well with the > existing usages of the macro: > > - For function objects: > > struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; > [...] > bpf_ksym_exists(bpf_task_acquire) > > - For variable objects: > > extern const struct rq runqueues __ksym __weak; /* typed */ > [...] > bpf_ksym_exists(&runqueues) > > Note also that BPF support was added in GCC 10 and support for > __builtin_has_attribute in GCC 9. It would be great if you can share details with asm code and BTF so we can understand better. I am not 100% sure about whether __builtin_has_attribute builtin can help to do run-time ksym resolution with libbpf. The following is what clang does: For example, for progs/test_ksyms_weak.c, we have 43 if (rq && bpf_ksym_exists(&runqueues)) 44 out__existing_typed = rq->cpu; ... 56 if (!bpf_ksym_exists(bpf_task_acquire)) 57 /* dead code won't be seen by the verifier */ 58 bpf_task_acquire(0); The asm code: .loc 0 42 20 prologue_end # progs/test_ksyms_weak.c:42:20 .Ltmp0: r6 = runqueues ll r1 = runqueues ll w2 = 0 call 153 .Ltmp1: .Ltmp2: #DEBUG_VALUE: pass_handler:rq <- $r0 .loc 0 43 9 # progs/test_ksyms_weak.c:43:9 .Ltmp3: if r0 == 0 goto LBB0_3 .Ltmp4: .Ltmp5: # %bb.1: # %entry #DEBUG_VALUE: pass_handler:rq <- $r0 if r6 == 0 goto LBB0_3 ... LBB0_5: # %if.end4 .loc 0 56 6 is_stmt 1 # progs/test_ksyms_weak.c:56:6 .Ltmp25: r1 = bpf_task_acquire ll if r1 != 0 goto LBB0_7 # %bb.6: # %if.then9 Here, 'runqueues' and 'bpf_task_acquire' will be changed by libbpf based on the *current* kernel state. The BTF datasec encodes such ksym information like below which will be used by libbpf: .long 13079 # BTF_KIND_DATASEC(id = 395) .long 251658247 # 0xf000007 .long 0 .long 377 .long bpf_task_acquire .long 0 .long 379 .long bpf_testmod_test_mod_kfunc .long 0 .long 381 .long invalid_kfunc .long 0 .long 387 .long runqueues .long 3264 .long 388 .long bpf_prog_active .long 1 .long 389 .long bpf_link_fops1 .long 1 .long 391 .long bpf_link_fops2 .long 4 What gcc generates for the above example? It would be great if this can be put in the commit message. > > Locally tested in bpf-next master branch. > No regressions. > > Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com> > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Cc: david.faust@oracle.com > Cc: cupertino.miranda@oracle.com > --- > tools/lib/bpf/bpf_helpers.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 62e1c0cc4a59..a720636a87d9 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -186,10 +186,19 @@ enum libbpf_tristate { > #define __kptr __attribute__((btf_type_tag("kptr"))) > #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr"))) > > +#if defined (__clang__) > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > !!sym; \ > }) > +#elif __GNUC__ > 8 | +#define bpf_ksym_exists(sym) ({ \ > + _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ > + !!sym; \ > +}) > +#else > +#define bpf_ksym_exists(sym) !!sym > +#endif > > #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx"))) > #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
> On 4/28/24 4:25 AM, Jose E. Marchesi wrote: >> The macro bpf_ksym_exists is defined in bpf_helpers.h as: >> >> #define bpf_ksym_exists(sym) ({ \ >> _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >> !!sym; \ >> }) >> >> The purpose of the macro is to determine whether a given symbol has >> been defined, given the address of the object associated with the >> symbol. It also has a compile-time check to make sure the object >> whose address is passed to the macro has been declared as weak, which >> makes the check on `sym' meaningful. >> >> As it happens, the check for weak doesn't work in GCC in all cases, >> because __builtin_constant_p not always folds at parse time when >> optimizing. This is because optimizations that happen later in the >> compilation process, like inlining, may make a previously non-constant >> expression a constant. This results in errors like the following when >> building the selftests with GCC: >> >> bpf_helpers.h:190:24: error: expression in static assertion is not constant >> 190 | _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Fortunately recent versions of GCC support a __builtin_has_attribute >> that can be used to directly check for the __weak__ attribute. This >> patch changes bpf_helpers.h to use that builtin when building with a >> recent enough GCC, and to omit the check if GCC is too old to support >> the builtin. >> >> The macro used for GCC becomes: >> >> #define bpf_ksym_exists(sym) ({ \ >> _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ >> !!sym; \ >> }) >> >> Note that since bpf_ksym_exists is designed to get the address of the >> object associated with symbol SYM, we pass *sym to >> __builtin_has_attribute instead of sym. When an expression is passed >> to __builtin_has_attribute then it is the type of the passed >> expression that is checked for the specified attribute. The >> expression itself is not evaluated. This accommodates well with the >> existing usages of the macro: >> >> - For function objects: >> >> struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; >> [...] >> bpf_ksym_exists(bpf_task_acquire) >> >> - For variable objects: >> >> extern const struct rq runqueues __ksym __weak; /* typed */ >> [...] >> bpf_ksym_exists(&runqueues) >> >> Note also that BPF support was added in GCC 10 and support for >> __builtin_has_attribute in GCC 9. > > It would be great if you can share details with asm code and > BTF so we can understand better. I am not 100% sure about > whether __builtin_has_attribute builtin can help to do > run-time ksym resolution with libbpf. Hi Yonghong. I am a bit confused. Is the _Static_assert supposed to contribute anything to the generated code? This is what GCC generates for pass_handler: ----- pass_handler: .LFB1: r2 = 0 r1 = runqueues ll call 153 if r0 == 0 goto .L2 r1 = runqueues ll if r1 == 0 goto .L2 r2 = out__existing_typed ll r0 = *(u32 *) (r0+2920) *(u32 *) (r2+0) = r0 .L2: r6 = out__non_existent_typed ll r1 = bpf_link_fops2 ll r3 = out__existing_typeless ll r4 = bpf_prog_active ll r5 = out__non_existent_typeless ll r9 = bpf_link_fops1 ll *(u64 *) (r3+0) = r4 *(u64 *) (r5+0) = r9 *(u64 *) (r6+0) = r1 if r1 == 0 goto .L3 r2 = 0 call 153 *(u64 *) (r6+0) = r0 .L3: r1 = bpf_task_acquire ll if r1 == 0 goto .L20 .L4: r1 = bpf_testmod_test_mod_kfunc ll if r1 == 0 goto .L21 .L5: r1 = invalid_kfunc ll if r1 == 0 goto .L6 call invalid_kfunc .L6: r0 = 0 exit .L21: call bpf_testmod_test_mod_kfunc goto .L5 .L20: call bpf_task_acquire goto .L4 .LFE1: .size pass_handler, .-pass_handler ----- And the .ksyms datasec: ----- [7693] DATASEC '.ksyms' size=0 vlen=7 type_id=7690 offset=0 size=0 (FUNC 'invalid_kfunc') type_id=7691 offset=0 size=0 (FUNC 'bpf_testmod_test_mod_kfunc') type_id=7692 offset=0 size=0 (FUNC 'bpf_task_acquire') type_id=7530 offset=0 size=4 (VAR 'bpf_link_fops2') type_id=7550 offset=0 size=1 (VAR 'bpf_link_fops1') type_id=7475 offset=0 size=1 (VAR 'bpf_prog_active') type_id=7535 offset=0 size=3456 (VAR 'runqueues') ----- Is the entry for runqueues en the datasec enough for libbpf to patch the ksym value in the corresponding `r1 = runqueues ll' instructions? > > The following is what clang does: > > For example, for progs/test_ksyms_weak.c, we have > 43 if (rq && bpf_ksym_exists(&runqueues)) > 44 out__existing_typed = rq->cpu; > ... > 56 if (!bpf_ksym_exists(bpf_task_acquire)) > 57 /* dead code won't be seen by the verifier */ > 58 bpf_task_acquire(0); > > The asm code: > > .loc 0 42 20 prologue_end # progs/test_ksyms_weak.c:42:20 > .Ltmp0: > r6 = runqueues ll > r1 = runqueues ll > w2 = 0 > call 153 > .Ltmp1: > .Ltmp2: > #DEBUG_VALUE: pass_handler:rq <- $r0 > .loc 0 43 9 # progs/test_ksyms_weak.c:43:9 > .Ltmp3: > if r0 == 0 goto LBB0_3 > .Ltmp4: > .Ltmp5: > # %bb.1: # %entry > #DEBUG_VALUE: pass_handler:rq <- $r0 > if r6 == 0 goto LBB0_3 > ... > LBB0_5: # %if.end4 > .loc 0 56 6 is_stmt 1 # progs/test_ksyms_weak.c:56:6 > .Ltmp25: > r1 = bpf_task_acquire ll > if r1 != 0 goto LBB0_7 > # %bb.6: # %if.then9 > > Here, 'runqueues' and 'bpf_task_acquire' will be changed by libbpf > based on the *current* kernel state. The BTF datasec encodes such ksym > information like below which will be used by libbpf: > > .long 13079 # BTF_KIND_DATASEC(id = 395) > .long 251658247 # 0xf000007 > .long 0 > .long 377 > .long bpf_task_acquire > .long 0 > .long 379 > .long bpf_testmod_test_mod_kfunc > .long 0 > .long 381 > .long invalid_kfunc > .long 0 > .long 387 > .long runqueues > .long 3264 > .long 388 > .long bpf_prog_active > .long 1 > .long 389 > .long bpf_link_fops1 > .long 1 > .long 391 > .long bpf_link_fops2 > .long 4 > > What gcc generates for the above example? It would be great > if this can be put in the commit message. > >> >> Locally tested in bpf-next master branch. >> No regressions. >> >> Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com> >> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> >> Cc: david.faust@oracle.com >> Cc: cupertino.miranda@oracle.com >> --- >> tools/lib/bpf/bpf_helpers.h | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> index 62e1c0cc4a59..a720636a87d9 100644 >> --- a/tools/lib/bpf/bpf_helpers.h >> +++ b/tools/lib/bpf/bpf_helpers.h >> @@ -186,10 +186,19 @@ enum libbpf_tristate { >> #define __kptr __attribute__((btf_type_tag("kptr"))) >> #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr"))) >> +#if defined (__clang__) >> #define bpf_ksym_exists(sym) ({ \ >> _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >> !!sym; \ >> }) >> +#elif __GNUC__ > 8 > > | +#define bpf_ksym_exists(sym) ({ \ > >> + _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ >> + !!sym; \ >> +}) >> +#else >> +#define bpf_ksym_exists(sym) !!sym >> +#endif >> #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx"))) >> #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
> .long runqueues > .long 3264 > .long 388 Just to be sure, there seems to be a little discrepancy. The size of runqueues (struct rq) in the latest bpf-next is 3456 instead of 3264, when compiled by both clang 18 and GCC. This is correctly reflected in the BTF generated by both compilers. Is the size of struct rq different in your testing machine?
On 5/2/24 10:44 AM, Jose E. Marchesi wrote: >> On 4/28/24 4:25 AM, Jose E. Marchesi wrote: >>> The macro bpf_ksym_exists is defined in bpf_helpers.h as: >>> >>> #define bpf_ksym_exists(sym) ({ \ >>> _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >>> !!sym; \ >>> }) >>> >>> The purpose of the macro is to determine whether a given symbol has >>> been defined, given the address of the object associated with the >>> symbol. It also has a compile-time check to make sure the object >>> whose address is passed to the macro has been declared as weak, which >>> makes the check on `sym' meaningful. >>> >>> As it happens, the check for weak doesn't work in GCC in all cases, >>> because __builtin_constant_p not always folds at parse time when >>> optimizing. This is because optimizations that happen later in the >>> compilation process, like inlining, may make a previously non-constant >>> expression a constant. This results in errors like the following when >>> building the selftests with GCC: >>> >>> bpf_helpers.h:190:24: error: expression in static assertion is not constant >>> 190 | _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> Fortunately recent versions of GCC support a __builtin_has_attribute >>> that can be used to directly check for the __weak__ attribute. This >>> patch changes bpf_helpers.h to use that builtin when building with a >>> recent enough GCC, and to omit the check if GCC is too old to support >>> the builtin. >>> >>> The macro used for GCC becomes: >>> >>> #define bpf_ksym_exists(sym) ({ \ >>> _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ >>> !!sym; \ >>> }) >>> >>> Note that since bpf_ksym_exists is designed to get the address of the >>> object associated with symbol SYM, we pass *sym to >>> __builtin_has_attribute instead of sym. When an expression is passed >>> to __builtin_has_attribute then it is the type of the passed >>> expression that is checked for the specified attribute. The >>> expression itself is not evaluated. This accommodates well with the >>> existing usages of the macro: >>> >>> - For function objects: >>> >>> struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; >>> [...] >>> bpf_ksym_exists(bpf_task_acquire) >>> >>> - For variable objects: >>> >>> extern const struct rq runqueues __ksym __weak; /* typed */ >>> [...] >>> bpf_ksym_exists(&runqueues) >>> >>> Note also that BPF support was added in GCC 10 and support for >>> __builtin_has_attribute in GCC 9. >> It would be great if you can share details with asm code and >> BTF so we can understand better. I am not 100% sure about >> whether __builtin_has_attribute builtin can help to do >> run-time ksym resolution with libbpf. > Hi Yonghong. > > I am a bit confused. Is the _Static_assert supposed to contribute > anything to the generated code? No it is not. It is used to check whether __weak attribute is added to the symbol or not. > > This is what GCC generates for pass_handler: > > ----- > pass_handler: > .LFB1: > r2 = 0 > r1 = runqueues ll > call 153 > if r0 == 0 goto .L2 > r1 = runqueues ll > if r1 == 0 goto .L2 > r2 = out__existing_typed ll > r0 = *(u32 *) (r0+2920) > *(u32 *) (r2+0) = r0 > .L2: > r6 = out__non_existent_typed ll > r1 = bpf_link_fops2 ll > r3 = out__existing_typeless ll > r4 = bpf_prog_active ll > r5 = out__non_existent_typeless ll > r9 = bpf_link_fops1 ll > *(u64 *) (r3+0) = r4 > *(u64 *) (r5+0) = r9 > *(u64 *) (r6+0) = r1 > if r1 == 0 goto .L3 > r2 = 0 > call 153 > *(u64 *) (r6+0) = r0 > .L3: > r1 = bpf_task_acquire ll > if r1 == 0 goto .L20 > .L4: > r1 = bpf_testmod_test_mod_kfunc ll > if r1 == 0 goto .L21 > .L5: > r1 = invalid_kfunc ll > if r1 == 0 goto .L6 > call invalid_kfunc > .L6: > r0 = 0 > exit > .L21: > call bpf_testmod_test_mod_kfunc > goto .L5 > .L20: > call bpf_task_acquire > goto .L4 > .LFE1: > .size pass_handler, .-pass_handler > ----- > > And the .ksyms datasec: > > ----- > [7693] DATASEC '.ksyms' size=0 vlen=7 > type_id=7690 offset=0 size=0 (FUNC 'invalid_kfunc') > type_id=7691 offset=0 size=0 (FUNC 'bpf_testmod_test_mod_kfunc') > type_id=7692 offset=0 size=0 (FUNC 'bpf_task_acquire') > type_id=7530 offset=0 size=4 (VAR 'bpf_link_fops2') > type_id=7550 offset=0 size=1 (VAR 'bpf_link_fops1') > type_id=7475 offset=0 size=1 (VAR 'bpf_prog_active') > type_id=7535 offset=0 size=3456 (VAR 'runqueues') > ----- > > Is the entry for runqueues en the datasec enough for libbpf to patch the > ksym value in the corresponding `r1 = runqueues ll' instructions It should be okay. libbpf will patch `r1 = runqueues ll` with `r1 = <btf_obj_fd/btf_id>` and the kernel will translate it to `r1 = <kernel addr of runqueues>`. Based on your above output, the patch looks good to me. Acked-by: Yonghong Song <yonghong.song@linux.dev> > >> The following is what clang does: >> >> For example, for progs/test_ksyms_weak.c, we have >> 43 if (rq && bpf_ksym_exists(&runqueues)) >> 44 out__existing_typed = rq->cpu; >> ... >> 56 if (!bpf_ksym_exists(bpf_task_acquire)) >> 57 /* dead code won't be seen by the verifier */ >> 58 bpf_task_acquire(0); >> >> The asm code: >> >> .loc 0 42 20 prologue_end # progs/test_ksyms_weak.c:42:20 >> .Ltmp0: >> r6 = runqueues ll >> r1 = runqueues ll >> w2 = 0 >> call 153 >> .Ltmp1: >> .Ltmp2: >> #DEBUG_VALUE: pass_handler:rq <- $r0 >> .loc 0 43 9 # progs/test_ksyms_weak.c:43:9 >> .Ltmp3: >> if r0 == 0 goto LBB0_3 >> .Ltmp4: >> .Ltmp5: >> # %bb.1: # %entry >> #DEBUG_VALUE: pass_handler:rq <- $r0 >> if r6 == 0 goto LBB0_3 >> ... >> LBB0_5: # %if.end4 >> .loc 0 56 6 is_stmt 1 # progs/test_ksyms_weak.c:56:6 >> .Ltmp25: >> r1 = bpf_task_acquire ll >> if r1 != 0 goto LBB0_7 >> # %bb.6: # %if.then9 >> >> Here, 'runqueues' and 'bpf_task_acquire' will be changed by libbpf >> based on the *current* kernel state. The BTF datasec encodes such ksym >> information like below which will be used by libbpf: >> >> .long 13079 # BTF_KIND_DATASEC(id = 395) >> .long 251658247 # 0xf000007 >> .long 0 >> .long 377 >> .long bpf_task_acquire >> .long 0 >> .long 379 >> .long bpf_testmod_test_mod_kfunc >> .long 0 >> .long 381 >> .long invalid_kfunc >> .long 0 >> .long 387 >> .long runqueues >> .long 3264 >> .long 388 >> .long bpf_prog_active >> .long 1 >> .long 389 >> .long bpf_link_fops1 >> .long 1 >> .long 391 >> .long bpf_link_fops2 >> .long 4 >> >> What gcc generates for the above example? It would be great >> if this can be put in the commit message. >> >>> Locally tested in bpf-next master branch. >>> No regressions. >>> >>> Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com> >>> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> >>> Cc: david.faust@oracle.com >>> Cc: cupertino.miranda@oracle.com >>> --- >>> tools/lib/bpf/bpf_helpers.h | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >>> index 62e1c0cc4a59..a720636a87d9 100644 >>> --- a/tools/lib/bpf/bpf_helpers.h >>> +++ b/tools/lib/bpf/bpf_helpers.h >>> @@ -186,10 +186,19 @@ enum libbpf_tristate { >>> #define __kptr __attribute__((btf_type_tag("kptr"))) >>> #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr"))) >>> +#if defined (__clang__) >>> #define bpf_ksym_exists(sym) ({ \ >>> _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >>> !!sym; \ >>> }) >>> +#elif __GNUC__ > 8 >> | +#define bpf_ksym_exists(sym) ({ \ >> >>> + _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ >>> + !!sym; \ >>> +}) >>> +#else >>> +#define bpf_ksym_exists(sym) !!sym >>> +#endif >>> #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx"))) >>> #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
On 5/2/24 11:23 AM, Jose E. Marchesi wrote: >> .long runqueues >> .long 3264 >> .long 388 > Just to be sure, there seems to be a little discrepancy. The size of > runqueues (struct rq) in the latest bpf-next is 3456 instead of 3264, > when compiled by both clang 18 and GCC. This is correctly reflected in > the BTF generated by both compilers. > > Is the size of struct rq different in your testing machine? Probably due to different configrations. Anyway, libbpf/kernel will do proper rewrite to reference to the proper kernel runqueus.
On Sun, Apr 28, 2024 at 4:26 AM Jose E. Marchesi <jose.marchesi@oracle.com> wrote: > > The macro bpf_ksym_exists is defined in bpf_helpers.h as: > > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > !!sym; \ > }) > > The purpose of the macro is to determine whether a given symbol has > been defined, given the address of the object associated with the > symbol. It also has a compile-time check to make sure the object > whose address is passed to the macro has been declared as weak, which > makes the check on `sym' meaningful. > > As it happens, the check for weak doesn't work in GCC in all cases, > because __builtin_constant_p not always folds at parse time when > optimizing. This is because optimizations that happen later in the > compilation process, like inlining, may make a previously non-constant > expression a constant. This results in errors like the following when > building the selftests with GCC: > > bpf_helpers.h:190:24: error: expression in static assertion is not constant > 190 | _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Fortunately recent versions of GCC support a __builtin_has_attribute > that can be used to directly check for the __weak__ attribute. This > patch changes bpf_helpers.h to use that builtin when building with a > recent enough GCC, and to omit the check if GCC is too old to support > the builtin. > > The macro used for GCC becomes: > > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ > !!sym; \ > }) > > Note that since bpf_ksym_exists is designed to get the address of the > object associated with symbol SYM, we pass *sym to > __builtin_has_attribute instead of sym. When an expression is passed > to __builtin_has_attribute then it is the type of the passed > expression that is checked for the specified attribute. The > expression itself is not evaluated. This accommodates well with the > existing usages of the macro: > > - For function objects: > > struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; > [...] > bpf_ksym_exists(bpf_task_acquire) > > - For variable objects: > > extern const struct rq runqueues __ksym __weak; /* typed */ > [...] > bpf_ksym_exists(&runqueues) > > Note also that BPF support was added in GCC 10 and support for > __builtin_has_attribute in GCC 9. > > Locally tested in bpf-next master branch. > No regressions. > > Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com> > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Cc: david.faust@oracle.com > Cc: cupertino.miranda@oracle.com > --- > tools/lib/bpf/bpf_helpers.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 62e1c0cc4a59..a720636a87d9 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -186,10 +186,19 @@ enum libbpf_tristate { > #define __kptr __attribute__((btf_type_tag("kptr"))) > #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr"))) > > +#if defined (__clang__) > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > !!sym; \ > }) > +#elif __GNUC__ > 8 > +#define bpf_ksym_exists(sym) ({ \ > + _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ > + !!sym; \ > +}) I wrapped _Static_assert() to keep it under 100 characters (and fix one unaligned '\' while at it). Also, the patch prefix should be "libbpf: " as this is purely a libbpf header. Applied to bpf-next, thanks. > +#else > +#define bpf_ksym_exists(sym) !!sym > +#endif > > #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx"))) > #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull"))) > -- > 2.30.2 > >
Hello: This patch was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <andrii@kernel.org>: On Sun, 28 Apr 2024 13:25:59 +0200 you wrote: > The macro bpf_ksym_exists is defined in bpf_helpers.h as: > > #define bpf_ksym_exists(sym) ({ \ > _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ > !!sym; \ > }) > > [...] Here is the summary with links: - [bpf-next] bpf: fix bpf_ksym_exists in GCC https://git.kernel.org/bpf/bpf-next/c/cf9bea94f6b2 You are awesome, thank you!
> On Sun, Apr 28, 2024 at 4:26 AM Jose E. Marchesi > <jose.marchesi@oracle.com> wrote: >> >> The macro bpf_ksym_exists is defined in bpf_helpers.h as: >> >> #define bpf_ksym_exists(sym) ({ \ >> _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >> !!sym; \ >> }) >> >> The purpose of the macro is to determine whether a given symbol has >> been defined, given the address of the object associated with the >> symbol. It also has a compile-time check to make sure the object >> whose address is passed to the macro has been declared as weak, which >> makes the check on `sym' meaningful. >> >> As it happens, the check for weak doesn't work in GCC in all cases, >> because __builtin_constant_p not always folds at parse time when >> optimizing. This is because optimizations that happen later in the >> compilation process, like inlining, may make a previously non-constant >> expression a constant. This results in errors like the following when >> building the selftests with GCC: >> >> bpf_helpers.h:190:24: error: expression in static assertion is not constant >> 190 | _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Fortunately recent versions of GCC support a __builtin_has_attribute >> that can be used to directly check for the __weak__ attribute. This >> patch changes bpf_helpers.h to use that builtin when building with a >> recent enough GCC, and to omit the check if GCC is too old to support >> the builtin. >> >> The macro used for GCC becomes: >> >> #define bpf_ksym_exists(sym) ({ \ >> _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ >> !!sym; \ >> }) >> >> Note that since bpf_ksym_exists is designed to get the address of the >> object associated with symbol SYM, we pass *sym to >> __builtin_has_attribute instead of sym. When an expression is passed >> to __builtin_has_attribute then it is the type of the passed >> expression that is checked for the specified attribute. The >> expression itself is not evaluated. This accommodates well with the >> existing usages of the macro: >> >> - For function objects: >> >> struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; >> [...] >> bpf_ksym_exists(bpf_task_acquire) >> >> - For variable objects: >> >> extern const struct rq runqueues __ksym __weak; /* typed */ >> [...] >> bpf_ksym_exists(&runqueues) >> >> Note also that BPF support was added in GCC 10 and support for >> __builtin_has_attribute in GCC 9. >> >> Locally tested in bpf-next master branch. >> No regressions. >> >> Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com> >> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> >> Cc: david.faust@oracle.com >> Cc: cupertino.miranda@oracle.com >> --- >> tools/lib/bpf/bpf_helpers.h | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> index 62e1c0cc4a59..a720636a87d9 100644 >> --- a/tools/lib/bpf/bpf_helpers.h >> +++ b/tools/lib/bpf/bpf_helpers.h >> @@ -186,10 +186,19 @@ enum libbpf_tristate { >> #define __kptr __attribute__((btf_type_tag("kptr"))) >> #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr"))) >> >> +#if defined (__clang__) >> #define bpf_ksym_exists(sym) ({ \ >> _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ >> !!sym; \ >> }) >> +#elif __GNUC__ > 8 >> +#define bpf_ksym_exists(sym) ({ \ >> + _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ >> + !!sym; \ >> +}) > > I wrapped _Static_assert() to keep it under 100 characters (and fix > one unaligned '\' while at it). Also, the patch prefix should be > "libbpf: " as this is purely a libbpf header. Applied to bpf-next, > thanks. Thank you. Sorry for the wrong prefix and for the style fixes. > >> +#else >> +#define bpf_ksym_exists(sym) !!sym >> +#endif >> >> #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx"))) >> #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull"))) >> -- >> 2.30.2 >> >>
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 62e1c0cc4a59..a720636a87d9 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -186,10 +186,19 @@ enum libbpf_tristate { #define __kptr __attribute__((btf_type_tag("kptr"))) #define __percpu_kptr __attribute__((btf_type_tag("percpu_kptr"))) +#if defined (__clang__) #define bpf_ksym_exists(sym) ({ \ _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ !!sym; \ }) +#elif __GNUC__ > 8 +#define bpf_ksym_exists(sym) ({ \ + _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ + !!sym; \ +}) +#else +#define bpf_ksym_exists(sym) !!sym +#endif #define __arg_ctx __attribute__((btf_decl_tag("arg:ctx"))) #define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))
The macro bpf_ksym_exists is defined in bpf_helpers.h as: #define bpf_ksym_exists(sym) ({ \ _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ !!sym; \ }) The purpose of the macro is to determine whether a given symbol has been defined, given the address of the object associated with the symbol. It also has a compile-time check to make sure the object whose address is passed to the macro has been declared as weak, which makes the check on `sym' meaningful. As it happens, the check for weak doesn't work in GCC in all cases, because __builtin_constant_p not always folds at parse time when optimizing. This is because optimizations that happen later in the compilation process, like inlining, may make a previously non-constant expression a constant. This results in errors like the following when building the selftests with GCC: bpf_helpers.h:190:24: error: expression in static assertion is not constant 190 | _Static_assert(!__builtin_constant_p(!!sym), #sym " should be marked as __weak"); \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ Fortunately recent versions of GCC support a __builtin_has_attribute that can be used to directly check for the __weak__ attribute. This patch changes bpf_helpers.h to use that builtin when building with a recent enough GCC, and to omit the check if GCC is too old to support the builtin. The macro used for GCC becomes: #define bpf_ksym_exists(sym) ({ \ _Static_assert(__builtin_has_attribute (*sym, __weak__), #sym " should be marked as __weak"); \ !!sym; \ }) Note that since bpf_ksym_exists is designed to get the address of the object associated with symbol SYM, we pass *sym to __builtin_has_attribute instead of sym. When an expression is passed to __builtin_has_attribute then it is the type of the passed expression that is checked for the specified attribute. The expression itself is not evaluated. This accommodates well with the existing usages of the macro: - For function objects: struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym __weak; [...] bpf_ksym_exists(bpf_task_acquire) - For variable objects: extern const struct rq runqueues __ksym __weak; /* typed */ [...] bpf_ksym_exists(&runqueues) Note also that BPF support was added in GCC 10 and support for __builtin_has_attribute in GCC 9. Locally tested in bpf-next master branch. No regressions. Signed-of-by: Jose E. Marchesi <jose.marchesi@oracle.com> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: david.faust@oracle.com Cc: cupertino.miranda@oracle.com --- tools/lib/bpf/bpf_helpers.h | 9 +++++++++ 1 file changed, 9 insertions(+)