On Wed, Jul 13, 2022 at 11:57 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On Wed, Jul 13, 2022 at 11:08 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Jul 13, 2022 at 9:24 AM <sdf@xxxxxxxxxx> wrote:
> > >
> > > On 07/12, Andrii Nakryiko wrote:
> > > > Libbpf supports single virtual __kconfig extern currently:
> > > > LINUX_KERNEL_VERSION.
> > > > LINUX_KERNEL_VERSION isn't coming from /proc/kconfig.gz and is intead
> > > > customly filled out by libbpf.
> > >
> > > > This patch generalizes this approach to support more such virtual
> > > > __kconfig externs. One such extern added in this patch is
> > > > LINUX_HAS_BPF_COOKIE which is used for BPF-side USDT supporting code in
> > > > usdt.bpf.h instead of using CO-RE-based enum detection approach for
> > > > detecting bpf_get_attach_cookie() BPF helper. This allows to remove
> > > > otherwise not needed CO-RE dependency and keeps user-space and BPF-side
> > > > parts of libbpf's USDT support strictly in sync in terms of their
> > > > feature detection.
> > >
> > > > We'll use similar approach for syscall wrapper detection for
> > > > BPF_KSYSCALL() BPF-side macro in follow up patch.
> > >
> > > > Generally, currently libbpf reserves CONFIG_ prefix for Kconfig values
> > > > and LINUX_ for virtual libbpf-backed externs. In the future we might
> > > > extend the set of prefixes that are supported. This can be done without
> > > > any breaking changes, as currently any __kconfig extern with
> > > > unrecognized name is rejected.
> > >
> > > > For LINUX_xxx externs we support the normal "weak rule": if libbpf
> > > > doesn't recognize given LINUX_xxx extern but such extern is marked as
> > > > __weak, it is not rejected and defaults to zero. This follows
> > > > CONFIG_xxx handling logic and will allow BPF applications to
> > > > opportunistically use newer libbpf virtual externs without breaking on
> > > > older libbpf versions unnecessarily.
> > >
> > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > > ---
> > > > tools/lib/bpf/libbpf.c | 69 +++++++++++++++++++++++++++++-----------
> > > > tools/lib/bpf/usdt.bpf.h | 16 ++--------
> > > > 2 files changed, 52 insertions(+), 33 deletions(-)
> > >
[...]
> > >
> > > > -/* don't rely on user's BPF code to have latest definition of
> > > > bpf_func_id */
> > > > -enum bpf_func_id___usdt {
> > > > - BPF_FUNC_get_attach_cookie___usdt = 0xBAD, /* value doesn't matter */
> > > > -};
> > > > +extern const _Bool LINUX_HAS_BPF_COOKIE __kconfig;
> > >
> > > Could _Bool be a problem when used by c++? I remember that we can have
> > > sizeof(bool) != sizeof(_Bool) when compiling with g++. If we were to
> > > use 'int' instead I'm assuming we'll loose all the niceties of
> > > KCFG_BOOL?
> > >
> > > Or shouldn't be a problem since it's not part of C/C++ api boundary?
> >
> > I actually don't know if C++ supports "_Bool", but in C, "bool" is an
> > alias to _Bool. _Bool is *the type* of the boolean. The benefit of
> > _Bool is that it doesn't require including stdbool.h, while "bool"
> > itself needs extra header. So I try not to use bool in libbpf BPF API
> > headers just to avoid extra header dependencies.
> >
> > But it shouldn't matter as this is BPF-side code, so it has to be
> > compiled in C mode by Clang/GCC, so _Bool should always be there.
>
> The program is fine, but these _Bool's can now leak into generated
> skeletons, right?
> And skeletons might be included into c++ and I'm not sure what happens
> with _Bool over there.
>
> My naive attempt to use it gives me the following:
>
> $ cat tst.cc
> int main(int argc, char *argv[])
> {
> _Bool have_args = argc > 1;
> if (have_args)
> return 1;
> else
> return 0;
> }
> $ clang++ tst.cc
> tst.cc:3:5: error: unknown type name '_Bool'
> _Bool have_args = argc > 1;
> ^
> 1 error generated.
Check test_cpp.cpp, it includes test_core_extern.skel.h, which has
bool externs in it. In generated code those bools are emitted as _Bool
(because that's what gets emitted to BTF by Clang). It seems to be
working fine with g++ already, and I just confirmed that clang++ also
works. So not sure what's different (perhaps stdbool.h included
somewhere "fixes" this for C++), but I don't it's the problem.
>
> > As for the size it seems like it's not even specified by the standard
> > that sizeof(bool/_Bool) is 1, though it is in practice. I only
> > remember some very-very-very old versions of Microsoft's Visual C++
> > having sizeof(bool) == 4, but then they changed that anyways to
> > sizeof(bool) == 1 (it was many-many years ago, so it might be an
> > incomplete story).
> >
> > But either way it doesn't matter, because libbpf will support any
> > size: 1, 2, 4, 8 and will just set 1 for true, 0 for false, with
> > correct zero extension to match variable size.
> >
> > As for bool vs int, no real difference, but it is true/false
> > conceptually, so seems cleaner to use bool. But using int will work
> > just fine here as well (you still get 0 or 1 for both, effectively).
>
> Yeah, so my question is more like: rather than trying to figure out if
> _Bool is safe, might it be easier to stick to ints?
>
I don't mind, but seems like it should be fine. It's not the first
bool used in skeleton, so if it didn't work, we'd be already suffering
and fixing it with some other means.
>
> > > > static __always_inline
> > > > int __bpf_usdt_spec_id(struct pt_regs *ctx)
> > > > {
> > > > - if (!BPF_USDT_HAS_BPF_COOKIE) {
> > > > + if (!LINUX_HAS_BPF_COOKIE) {
> > > > long ip = PT_REGS_IP(ctx);
> > > > int *spec_id_ptr;
> > >
> > > > --
> > > > 2.30.2
> > >
