search

libbpf / bpftool

Automated upstream mirror for bpftool stand-alone build.
Other
391 stars 71 forks source link

libbfd feature broken due to inconsistent feature detection between kernel tree (ok) and GH repo (broken) #166

Open hhoffstaette opened 3 days ago

hhoffstaette commented 3 days ago

Reporting an issue

Environment

Describe the bug

I'm trying to switch the Gentoo bpftool build from a kernel-tree based build to the Github package. This existing build works fine and can properly enable features like choosing between libbfd or llvm e.g. for JIT disassembly, and both are verified to work (we default to bfd).

This does not work with the Github tree because the libbfd detection is broken.

Building with LLVM as preferred backend works:

$make feature-libbfd=0 feature-llvm=1      
...                        libbfd: [ OFF ]
...               clang-bpf-co-re: [ on  ]
...                          llvm: [ on  ]
...                        libcap: [ on  ]

The resulting bpftool is linked against the correct llvm version and can dump a jited program. However this does not work when choosing bfd:

$make feature-libbfd=1 feature-llvm=0      
...                        libbfd: [ on ]
...               clang-bpf-co-re: [ on  ]
...                          llvm: [ OFF ]
...                        libcap: [ on  ]
<snip>
In file included from jit_disasm.c:37:
/home/holger/tmp/bpftool/include/tools/dis-asm-compat.h:10:6: error: redeclaration of 'enum disassembler_style'
   10 | enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
      |      ^~~~~~~~~~~~~~~~~~
In file included from jit_disasm.c:36:
/usr/include/dis-asm.h:53:6: note: originally defined here
   53 | enum disassembler_style
      |      ^~~~~~~~~~~~~~~~~~
/home/holger/tmp/bpftool/include/tools/dis-asm-compat.h: In function 'init_disassemble_info_compat':
/home/holger/tmp/bpftool/include/tools/dis-asm-compat.h:50:9: error: too few arguments to function 'init_disassemble_info'
   50 |         init_disassemble_info(info, stream,
      |         ^~~~~~~~~~~~~~~~~~~~~
/usr/include/dis-asm.h:482:13: note: declared here
  482 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
      |             ^~~~~~~~~~~~~~~~~~~~~
jit_disasm.c: In function 'init_context':
jit_disasm.c:284:41: error: incompatible type for argument 1 of 'disassembler'
  284 |         ctx->disassemble = disassembler(bfdf);
      |                                         ^~~~
      |                                         |
      |                                         bfd *
/usr/include/dis-asm.h:411:63: note: expected 'enum bfd_architecture' but argument is of type 'bfd *'
  411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
      |                                         ~~~~~~~~~~~~~~~~~~~~~~^~~
jit_disasm.c:284:28: error: too few arguments to function 'disassembler'
  284 |         ctx->disassemble = disassembler(bfdf);
      |                            ^~~~~~~~~~~~
/usr/include/dis-asm.h:411:27: note: declared here
  411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
      |                           ^~~~~~~~~~~~
make: *** [Makefile:243: jit_disasm.o] Error 1

Building with V=1 shows a familiar problem:

Makefile.feature:54: Probing: feature-libbfd
Makefile.feature:55: printf '%b\n' '#include <bfd.h>\n' 'int main(void) {' '    bfd_demangle(0, 0, 0);' '   return 0;' '}' | cc  -Wall -Werror -x c - -lbfd -ldl -o /dev/null >/dev/null && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
In file included from /usr/include/bfd.h:10,
                 from <stdin>:1:
/usr/include/x86_64-pc-linux-gnu/bfd.h:35:2: error: #error config.h must be included before this header
   35 | #error config.h must be included before this header
      |  ^~~~~
result: 0

This works properly in the kernel tree:

$tar xf linux-6.11.3.tar.xz
$cd linux-6.11.3/tools/bpf/bpftool                    
$make V=1 feature-libbfd=1 feature-llvm=0                  

Auto-detecting system features:
...                         clang-bpf-co-re: [ on  ]
...                                    llvm: [ OFF ]
...                                  libcap: [ on  ]
...                                  libbfd: [ on  ]

<build continues and bpftool has the expected libbfd dependency>

I cannot tell whether this is expected or I'm doing something wrong.

qmonnet commented 3 days ago

Hi Holger, thank you for the report!

I cannot tell whether this is expected or I'm doing something wrong.

I would say, this is “undefined behaviour” :)

Bpftool's Makefile does not support what you're trying to do: it does not let you pick the features you want. It just picks up the features based on what's available (and defaults to LLVM for the disassembler). Passing the feature-* may work in the kernel repo, it may work with some combinations of options in the mirror, but it's not supposed to be used this way.

What happens when you call make feature-libbfd=1 feature-llvm=0 is that you're not requiring these features, you're overriding the feature detection and telling the Makefile that these are supported (or not). It sounds acceptable if you're confident of the availability of the bfd or llvm libraries on the building system. But what you don't see in that example is that feature-libbfd is one of the different probes that the Makefile does for libbfd under the hood. Just for checking libbfd's availability, there's feature-libbfd, feature-libbfd-liberty, and feature-libbfd-liberty-z, and the bfd disassembler is picked if one of the three is enabled. I'm not 100% sure but I suspect that in your case, you get the error because the Makefile should use one of the last two instead of feature-libbfd. I'm not entirely sure why you don't get the issue in the kernel repo, but then feature detection doesn't work exactly the same so feature-libbfd might work anyway.

So we don't get a way to choose the options with bpftool's Makefile. We considered having a way to disable some features in the past, but eventually decided against it. One thing to note is that when we had this discussion, only the libbfd assembler was available in bpftool. Then I made the one based on LLVM the default for a number of reasons, including that dynamic linking against libbfd is not allowed in Debian. But it would make sense to offer a choice to the user between the two disassemblers, without having to remove the llvm lib from the system.

And the two options I see in your case would be:

hhoffstaette commented 3 days ago

Thanks for the reply Quentin. Not sure where to go from here but this is .. unexpected.

While I understand that the whole problem is difficult due to platform differences & build combinatons, even make feature-llvm=0 and using the fallback logic does not work as expected, as it does not link to bfd and consequently does not have JIT dump support (because the feature check still failed). I tried all three feature-bfd settings and none of them change the fact that the detection fails due to the bfd include guard. I tried setting PACKAGE_VERSION as workaround to silence the bfd include guard but this didn't work either (probably did it wrong).

Not sure what to do wrt. skipping the broken feature detection but will investigate - thanks for the pointer. I'm not afraid of taking a creative sed chainsaw to the Makefile to enable/disable options, but would rather not do so if it can be avoided.

hhoffstaette commented 3 days ago

I chose violence:

diff --git a/src/Makefile.feature b/src/Makefile.feature
index 131c67e..f31ad6b 100644
--- a/src/Makefile.feature
+++ b/src/Makefile.feature
@@ -39,7 +39,9 @@ endif # clang-bpf-co-re
 ### feature-libbfd

 ifneq ($(findstring libbfd,$(FEATURE_TESTS)),)
-LIBBFD_PROBE := '$(pound)include <bfd.h>\n'
+LIBBFD_PROBE := '$(pound)define PACKAGE\n'
+LIBBFD_PROBE += '$(pound)include <bfd.h>\n'
+LIBBFD_PROBE += '$(pound)undef PACKAGE\n'
 LIBBFD_PROBE += 'int main(void) {'
 LIBBFD_PROBE += '  bfd_demangle(0, 0, 0);'
 LIBBFD_PROBE += '  return 0;'
@@ -71,7 +73,9 @@ endif # libbfd
 ### feature-disassembler-four-args

 ifneq ($(findstring disassembler-four-args,$(FEATURE_TESTS)),)
-DISASSEMBLER_PROBE := '$(pound)include <dis-asm.h>\n'
+DISASSEMBLER_PROBE := '$(pound)define PACKAGE\n'
+DISASSEMBLER_PROBE += '$(pound)include <dis-asm.h>\n'
+DISASSEMBLER_PROBE += '$(pound)undef PACKAGE\n'
 DISASSEMBLER_PROBE += 'int main(void) {'
 DISASSEMBLER_PROBE += '    disassembler((enum bfd_architecture)0, 0, 0, NULL);'
 DISASSEMBLER_PROBE += '    return 0;'
@@ -91,7 +95,9 @@ endif # disassembler-four-args
 ### feature-disassembler-init-styled

 ifneq ($(findstring disassembler-init-styled,$(FEATURE_TESTS)),)
-DISASSEMBLER_STYLED_PROBE := '$(pound)include <dis-asm.h>\n'
+DISASSEMBLER_STYLED_PROBE := '$(pound)define PACKAGE\n'
+DISASSEMBLER_STYLED_PROBE += '$(pound)include <dis-asm.h>\n'
+DISASSEMBLER_STYLED_PROBE += '$(pound)undef PACKAGE\n'
 DISASSEMBLER_STYLED_PROBE += 'int main(void) {'
 DISASSEMBLER_STYLED_PROBE += ' init_disassemble_info(NULL, 0, NULL, NULL);'
 DISASSEMBLER_STYLED_PROBE += ' return 0;'

Then:

$make -j8 V=1 feature-libbfd=1 feature-llvm=0
holger>make -j8 V=1 feature-libbfd=1 feature-llvm=0
Makefile.feature:34: Probing: feature-clang-bpf-co-re
Makefile.feature:35: printf '%s\n' 'struct s { int i; } __attribute__((preserve_access_index)); struct s foo = {};' | clang -g -target bpf -S -o - -x c -  | grep -q BTF_KIND_VAR && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
result: 1
Makefile.feature:56: Probing: feature-libbfd
Makefile.feature:57: printf '%b\n' '#define PACKAGE\n' '#include <bfd.h>\n' '#undef PACKAGE\n' 'int main(void) {' ' bfd_demangle(0, 0, 0);' '   return 0;' '}' | cc  -Wall -Werror -x c - -lbfd -ldl -o /dev/null >/dev/null && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
result: 1
Makefile.feature:90: Probing: feature-disassembler-four-args
Makefile.feature:91: printf '%b\n' '#define PACKAGE\n' '#include <dis-asm.h>\n' '#undef PACKAGE\n' 'int main(void) {' ' disassembler((enum bfd_architecture)0, 0, 0, NULL);' '  return 0;' '}' | cc  -Wall -Werror -x c - -lbfd -lopcodes -S -o - >/dev/null && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
result: 1
Makefile.feature:106: Probing: feature-disassembler-styled
Makefile.feature:107: printf '%b\n' '#define PACKAGE\n' '#include <dis-asm.h>\n' '#undef PACKAGE\n' 'int main(void) {' '    init_disassemble_info(NULL, 0, NULL, NULL);' '  return 0;' '}' | cc  -Wall -Werror -x c - -lbfd -lopcodes -S -o - >/dev/null && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
result: 1
Makefile.feature:126: Probing: feature-libcap
Makefile.feature:127: printf '%b\n' '#include <sys/capability.h>\n' 'int main(void) {' '    cap_free(0);' ' return 0;' '}' | cc  -Wall -Werror -x c - -lcap -S -o - >/dev/null && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
result: 1
Makefile.feature:164: Probing: feature-llvm
Makefile.feature:165: printf '%b\n' '#include <llvm-c/Core.h>\n' '#include <llvm-c/TargetMachine.h>\n' 'int main(void) {' ' char *triple = LLVMNormalizeTargetTriple("");' '    LLVMDisposeMessage(triple);' '  return 0;' '}' | cc  -I/usr/lib/llvm/19/include  -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS  -L/usr/lib/llvm/19/lib64  -Wall -Werror -x c - -lLLVM-19 -o /dev/null >/dev/null && (echo 1 && >&2 echo result: 1) || (echo 0 && >&2 echo result: 0)
result: 1
...                        libbfd: [ on  ]
...               clang-bpf-co-re: [ on  ]
...                          llvm: [ OFF ]
...                        libcap: [ on  ]
<snip>

The build not only completes, but:

$ldd bpftool
    linux-vdso.so.1 (0x00007f6708e17000)
    libelf.so.1 => /usr/lib64/libelf.so.1 (0x00007f6708d31000)
    libz.so.1 => /usr/lib64/libz.so.1 (0x00007f6708d17000)
    libcap.so.2 => /usr/lib64/libcap.so.2 (0x00007f6708d0b000)
    libbfd-2.43.1.gentoo-sys-libs-binutils-libs-st-def.so => /usr/lib64/libbfd-2.43.1.gentoo-sys-libs-binutils-libs-st-def.so (0x00007f6708bda000)
    libopcodes-2.43.1.gentoo-sys-libs-binutils-libs-st-def.so => /usr/lib64/libopcodes-2.43.1.gentoo-sys-libs-binutils-libs-st-def.so (0x00007f6708b23000)
    libc.so.6 => /lib64/libc.so.6 (0x00007f670892c000)
    libzstd.so.1 => /usr/lib64/libzstd.so.1 (0x00007f6708865000)
    /lib64/ld-linux-x86-64.so.2 (0x00007f6708e19000)
    libsframe.so.1 => /usr/lib64/libsframe.so.1 (0x00007f670885c000)

..and sudo ./bpftool prog dump jited name mybpfprogram shows me some sweet bytecode. \o/

qmonnet commented 3 days ago

Great to have it fixed! :slightly_smiling_face:

I'm so sorry re-reading your question and my answer I think I just didn't understand the issue. It's probably not because of the way you're passing the arguments to the Makefile, but just the regular detection for libbfd that fails on your system: a simple make would show libbfd: off even though the lib is installed, wouldn't it?

So sorry for all the considerations above :facepalm:. We “just” need to fix detection for libbfd in your case. Which you managed to do by defining PACKAGE, but I'm curious to understand why I don't need it on my system. I'll take a look.

hhoffstaette commented 3 days ago

a simple make would show libbfd: off even though the lib is installed, wouldn't it?

Precisely. It's not that I could not enable it, it's that bfd did not work at all.

hhoffstaette commented 3 days ago

One of the reasons this is important is that we want to eventually enable use of a purely gcc-based toolchain to have an alternative BPF compiler to clang.

hhoffstaette commented 3 days ago

So this is why it works in the kernel build: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/build/feature/Makefile#n114

qmonnet commented 3 days ago

Yes indeed. I also noticed there are several versions of bfd.h in the wild, I've a system with:

/* PR 14072: Ensure that config.h is included first.  */
#if !defined PACKAGE && !defined PACKAGE_VERSION
#error config.h must be included before this header
#endif

but this snippet is absent from /usr/include/bfd.h in Ubuntu, for example. I haven't been able to find where this has been removed so far, but it looks like a distro thing to me.

Let's add a -DPACKAGE='"bpftool"' to the feature detection on the mirror. I'll look into it tonight if I can.

hhoffstaette commented 2 days ago

Let's add a -DPACKAGE='"bpftool"' to the feature detection on the mirror. I'll look into it tonight if I can.

Doing exactly that right now :rocket: 

diff --git a/src/Makefile.feature b/src/Makefile.feature
index 131c67e..794aec5 100644
--- a/src/Makefile.feature
+++ b/src/Makefile.feature
@@ -4,6 +4,7 @@ pound := \#

 CFLAGS_BACKUP := $(CFLAGS)
 CFLAGS := $(EXTRA_CFLAGS)
+CFLAGS += -DPACKAGE='"bpftool"'
 ifneq ($(LLVM),)
   CFLAGS += -Wno-unused-command-line-argument
 endif

Works as advertised. Now I also need to explicitly disable all bfd options to get a pure llvm build, which AFAIU is the expected and correct behaviour as well:

$make -j8 V=1 feature-libbfd=0 feature-libbfd-liberty=0 feature-libbfd-liberty-z=0 feature-llvm=1
<snip>
...                        libbfd: [ OFF ]
...               clang-bpf-co-re: [ on  ]
...                          llvm: [ on  ]
...                        libcap: [ on  ]

That being said this is not necessary as the build will still prefer llvm when enabled, so that's good.