From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org,
Alexei Starovoitov <alexei.starovoitov@gmail.com>,
david.faust@oracle.com, cupertino.miranda@oracle.com
Subject: Re: [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC
Date: Fri, 03 May 2024 09:50:53 +0200 [thread overview]
Message-ID: <87bk5npc4i.fsf@oracle.com> (raw)
In-Reply-To: <CAEf4Bzb-bbEZ5Q6vSX+tiMu4iME2uVjN1T3d3vPZXMe5ngAfxQ@mail.gmail.com> (Andrii Nakryiko's message of "Thu, 2 May 2024 22:52:24 -0700")
> 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
>>
>>
next prev parent reply other threads:[~2024-05-03 7:51 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-28 11:25 [PATCH bpf-next] bpf: fix bpf_ksym_exists in GCC Jose E. Marchesi
2024-04-29 20:52 ` Yonghong Song
2024-05-02 17:44 ` Jose E. Marchesi
2024-05-03 4:56 ` Yonghong Song
2024-05-02 18:23 ` Jose E. Marchesi
2024-05-03 4:58 ` Yonghong Song
2024-05-03 5:52 ` Andrii Nakryiko
2024-05-03 7:50 ` Jose E. Marchesi [this message]
2024-05-03 6:00 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bk5npc4i.fsf@oracle.com \
--to=jose.marchesi@oracle.com \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=cupertino.miranda@oracle.com \
--cc=david.faust@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).