Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cannot build with bcc 0.25.0 #2340

Closed
martinetd opened this issue Aug 24, 2022 · 9 comments
Closed

cannot build with bcc 0.25.0 #2340

martinetd opened this issue Aug 24, 2022 · 9 comments
Labels
bug Something isn't working

Comments

@martinetd
Copy link
Contributor

bcc changed the prototype of bcc_prog_load_xattr in 0.25.0:

commit 185143bdec6134255363446f644acd766ffb3825
Author: chantra <chantr4@gmail.com>
Date:   Tue Jul 5 22:51:55 2022 +0000

    [bcc] stop using deprecated `bpf_load_program_attr`
    
    https://github.com/libbpf/libbpf/commit/9476dce6fe905a6bf1d4c483f7b2b8575c4ffb2d remove it from libbpf

diff --git a/src/cc/libbpf.h b/src/cc/libbpf.h
index c5ea40a505a0..dd86f0a9544f 100644
--- a/src/cc/libbpf.h
+++ b/src/cc/libbpf.h
@@ -88,10 +89,11 @@ int bcc_prog_load(enum bpf_prog_type prog_type, const char *name,
                   const struct bpf_insn *insns, int prog_len,
                   const char *license, unsigned kern_version,
                   int log_level, char *log_buf, unsigned log_buf_size);
-int bcc_prog_load_xattr(struct bpf_load_program_attr *attr,
+int bcc_prog_load_xattr(enum bpf_prog_type prog_type, const char *prog_name,
+						const char *license, const struct bpf_insn *insns,
+						struct bpf_prog_load_opts *opts,
                         int prog_len, char *log_buf,
                         unsigned log_buf_size, bool allow_rlimit);
-
 int bpf_attach_socket(int sockfd, int progfd);
 
 /* create RAW socket. If name is not NULL/a non-empty null-terminated string,

and bpftrace cannot cope with that yet:

/home/builder/bpftrace/src/bpftrace-0.15.0/src/attached_probe.cpp: In member function 'void bpftrace::AttachedProbe::load_prog()':
/home/builder/bpftrace/src/bpftrace-0.15.0/src/attached_probe.cpp:752:11: error: cannot convert 'bpf_load_program_attr*' to 'bpf_prog_type'
  752 |           &attr, prog_len, log_buf.get(), log_buf_size, true);
      |           ^~~~~
      |           |
      |           bpf_load_program_attr*
In file included from /home/builder/bpftrace/src/bpftrace-0.15.0/src/bpffeature.h:3,
                 from /home/builder/bpftrace/src/bpftrace-0.15.0/src/attached_probe.h:8,
                 from /home/builder/bpftrace/src/bpftrace-0.15.0/src/attached_probe.cpp:24:
/usr/include/bcc/libbpf.h:92:44: note:   initializing argument 1 of 'int bcc_prog_load_xattr(bpf_prog_type, const char*, const char*, const bpf_insn*, bpf_prog_load_opts*, int, char*, unsigned int, bool)'
   92 | int bcc_prog_load_xattr(enum bpf_prog_type prog_type, const char *prog_name,
      |                         ~~~~~~~~~~~~~~~~~~~^~~~~~~~~

I'll open a PR to add a cmake check and use either prototype when I can find time for it, but if someone else wants to beat me to it feel free to (I'll repost here when I start, and it likely won't be this week)

@martinetd martinetd added the bug Something isn't working label Aug 24, 2022
@viktormalik
Copy link
Contributor

Just FYI, we'll be dropping dependency on BCC for program loading (see #2334). But this fix is still useful to enable builds against bcc 0.25 in the meantime.

@martinetd
Copy link
Contributor Author

Thanks for the heads up, fc940d3 indeed removes that call to bcc_prog_load_xattr() so it does actually fix this problem, and the current master branch builds with bcc 0.25.0!

Unfortunately it's also totally impossible to backport to 0.15.0 -- so I'm not sure what we want to do here...
alpine edge is left with bpftrace not working because they upgraded bcc first without checking, and nixos is probably going to wait on bpftrace for the libbpf 1.0.0 upgrade I guess?

Either way a quick solution might be better for distros... But bpftrace doesn't do stable branches really do you?
Would you plan on cutting a 0.16.0 soon?

Meanwhile I was totally blind and actually already have a patch on top of 0.15.0 that does just what we need, but I'd rather not use it if we can get away with a new release :)

(patch below applies on 0.15.0)

From f86c3bde84d9e75ec2780a51a1ca7dc20a4740e6 Mon Sep 17 00:00:00 2001
From: Dominique Martinet <asmadeus@codewreck.org>
Date: Thu, 25 Aug 2022 21:47:30 +0900
Subject: [PATCH] Fix builds against bcc >= 0.25.0

libbpf 1.0.0 removed bpf_load_program_attr in
https://github.com/libbpf/libbpf/commit/9476dce6fe905a6bf1d4c483f7b2b8575c4ffb2d
and bcc 0.25.0 in turn changed bcc_prog_load_xattr to use
bpf_prog_load_opts instead in
https://github.com/iovisor/bcc/commit/185143bdec6134255363446f644acd766ffb3825

Add a compile check to use the appropriate version

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c5c4c39630d2..f23b2ba52740 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -138,7 +138,7 @@ check_symbol_exists(bpf_attach_kfunc "${LIBBCC_INCLUDE_DIRS}/bcc/libbpf.h" HAVE_
 check_symbol_exists(bcc_usdt_addsem_probe "${LIBBCC_INCLUDE_DIRS}/bcc/bcc_usdt.h" HAVE_BCC_USDT_ADDSEM)
 check_symbol_exists(bcc_procutils_which_so "${LIBBCC_INCLUDE_DIRS}/bcc/bcc_proc.h" HAVE_BCC_WHICH_SO)
 
-# bcc_prog_load_xattr needs struct bpf_load_program_attr,
+# bcc_prog_load_xattr needs struct bpf_prog_load_opts or bpf_load_program_attr,
 # which is defined in libbpf
 if (LIBBPF_FOUND)
   check_symbol_exists(bcc_prog_load_xattr "${LIBBCC_INCLUDE_DIRS}/bcc/libbpf.h" HAVE_BCC_PROG_LOAD_XATTR)
@@ -236,6 +236,10 @@ if(LIBBCC_ATTACH_UPROBE_SEVEN_ARGS_SIGNATURE)
   set(BPFTRACE_FLAGS "${BPFTRACE_FLAGS}" LIBBCC_ATTACH_UPROBE_SEVEN_ARGS_SIGNATURE)
 endif(LIBBCC_ATTACH_UPROBE_SEVEN_ARGS_SIGNATURE)
 
+if(LIBBCC_PROG_LOAD_XATTRS_WITH_OPTS)
+  set(BPFTRACE_FLAGS "${BPFTRACE_FLAGS}" LIBBCC_PROG_LOAD_XATTRS_WITH_OPTS)
+endif(LIBBCC_PROG_LOAD_XATTRS_WITH_OPTS)
+
 if (HAVE_BCC_KFUNC)
   set(BPFTRACE_FLAGS "${BPFTRACE_FLAGS}" HAVE_BCC_KFUNC)
 endif(HAVE_BCC_KFUNC)
diff --git a/cmake/FindLibBcc.cmake b/cmake/FindLibBcc.cmake
index 7b4f12835786..20d2e68cd75f 100644
--- a/cmake/FindLibBcc.cmake
+++ b/cmake/FindLibBcc.cmake
@@ -85,6 +85,16 @@ int main(void) {
   return 0;
 }
 " LIBBCC_ATTACH_UPROBE_SEVEN_ARGS_SIGNATURE)
+CHECK_CXX_SOURCE_COMPILES("
+#include <bcc/libbpf.h>
+
+int main(void) {
+  struct bpf_prog_load_opts *opts = (struct bpf_prog_load_opts*) 1;
+
+  bcc_prog_load_xattr(BPF_PROG_TYPE_UNSPEC, 0, 0, 0, opts, 0, 0, 0, true);
+  return 0;
+}
+" LIBBCC_PROG_LOAD_XATTRS_WITH_OPTS)
 SET(CMAKE_REQUIRED_INCLUDES)
 
 SET(CMAKE_REQUIRED_LIBRARIES ${LIBBCC_BPF_LIBRARIES})
diff --git a/src/attached_probe.cpp b/src/attached_probe.cpp
index 60778e53ce44..dd46f15fd8d2 100644
--- a/src/attached_probe.cpp
+++ b/src/attached_probe.cpp
@@ -731,7 +731,24 @@ void AttachedProbe::load_prog()
         continue;
       }
 
-#ifdef HAVE_BCC_PROG_LOAD_XATTR
+#ifdef LIBBCC_PROG_LOAD_XATTRS_WITH_OPTS
+    struct bpf_prog_load_opts opts = { };
+
+    opts.sz = sizeof(opts);
+    opts.log_level = log_level;
+
+    progfd_ = bcc_prog_load_xattr(
+        progtype(probe_.type),
+        name.c_str(),
+        license,
+        reinterpret_cast<struct bpf_insn *>(insns),
+        &opts,
+        prog_len,
+        log_buf.get(),
+        log_buf_size,
+        true);
+
+#elif HAVE_BCC_PROG_LOAD_XATTR
       struct bpf_load_program_attr attr = {};
 
       attr.prog_type = progtype(probe_.type);

@fbs
Copy link
Contributor

fbs commented Aug 25, 2022

I wanted to keep a 3 month-ish release cycle for problems like these, so if it helps we can do the 0.16 release soon. Depends on what is currently in flight

@viktormalik
Copy link
Contributor

We have two bigger features (#2315 and #2321) but they may still take some time, so I wouldn't block a release with them. Also, we haven't released the join fix which blocks one of the tools, yet, so let's wait for #2322 and #2343 to land (which should be quick) and release 0.16.

@martinetd
Copy link
Contributor Author

Thanks! It would be great -- I'll let you handle it then :)

(master branch builds fine with bcc 0.25.0 so this issue could technically be closed, but it'll be slightly easier to follow if we close it when release is made -- no hard preference though, please close if appropriate...)

@anatol
Copy link

anatol commented Aug 29, 2022

Arch Linux hit the same issue with bcc 0.25 vs bpftrace https://bugs.archlinux.org/task/75718

@martinetd
Copy link
Contributor Author

martinetd commented Aug 30, 2022

Arch Linux hit the same issue with bcc 0.25 vs bpftrace bugs.archlinux.org/task/75718

FWIW since alpine also already had bcc 0.25 in the repos and bpftrace was unusable I went with my compatibility patch on 0.15 for now: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/38080

We'll upgrade to bpftrace 0.16 as soon as it's released.

(for my nixpkgs hat, we'll either keep both versions around until bpftrace 0.16 is released or just delay the upgrade of the whole stack as there are extra problems with e.g. systemd and libbpf 1.0 we just found out)

@viktormalik
Copy link
Contributor

@fbs I think that we're good to do a release

@martinetd
Copy link
Contributor Author

martinetd commented Aug 31, 2022

Thanks! 0.16.0 looks good :)

alpine upgrade MR: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/38438
nixos upgrade PR: NixOS/nixpkgs#189078

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants