[PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Jose E. Marchesi]

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;												\
+})
+#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

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Yonghong Song]

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

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Jose E. Marchesi]

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

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Jose E. Marchesi]

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

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Yonghong Song]

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

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Yonghong Song]

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.

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Andrii Nakryiko]

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

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC

[Jose E. Marchesi]

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