BTF kernel code support #734

olsajiri · 2019-06-07T12:35:02Z

Adding support to use BTF kernel data to access kernel fields.

This is a follow up on recent RFC pull request addressing the
build issues and adding automated tests.

The bpftrace now detects BTF data and if present it will use it
to acess kernel related fields. With this patchset it's possible to
run following:

# bpftrace -e 'kprobe:do_dentry_open { printf("%s %d - %s %d - %s %d\n", \
                   curtask->comm, curtask->pid, \
                   curtask->parent->comm, curtask->parent->pid, \
                   curtask->parent->parent->comm, curtask->parent->parent->pid); }'
Attaching 1 probe...
chronyd 1102 - systemd 1 - swapper/0 0
bash 14311 - bash 14207 - su 14203
bash 14311 - bash 14207 - su 14203
ls 14311 - bash 14207 - su 14203
ls 14311 - bash 14207 - su 14203
systemd-journal 861 - systemd 1 - swapper/0 0
NetworkManager 1098 - systemd 1 - swapper/0 0
sssd 1099 - systemd 1 - swapper/0 0
NetworkManager 1098 - systemd 1 - swapper/0 0
NetworkManager 14312 - NetworkManager 1098 - systemd 1

The bpftrace build now detects libbpf devel package (present in Fedora 30),
and enables the btf code accordingly. It's switched off for static build now.

To use it, you still need to generate BTF kernel data manualy. I plan for
Fedora kernel to package this soon, but for now, you need to do following:

get latest dwarves with pahole v1.13
enable kernel config CONFIG_DEBUG_INFO_BTF=y and build new kernel
get its BTF data: $ objcopy --dump-section .BTF=./btf ./vmlinux
copy the btf file under /lib/modules/``uname -r``/btf
or run bpftrace with BPFTRACE_BTF=./btf bpftrace ...

danobi · 2019-06-26T10:36:10Z

I believe @anakryiko has been working on a tool that converts BTF to a standard C header file. I wonder if we could reuse any of that.

Andrii, do you have any details you could share?

anakryiko · 2019-06-26T18:34:01Z

@danobi, yes, it is already available as part of bpftool. You can just do:

bpftool btf dump file <path to kernel image with .BTF section> format c

to compilable .h file.

As for the description of this change, I'm wondering why all those steps to dump .BTF section into separate file is needed? Why can't you just point to kernel image w/ .BTF and use libbpf to extract BTF data using existing API. No hassle and it should just work.

olsajiri · 2019-06-26T19:20:20Z

@danobi, yes, it is already available as part of bpftool. You can just do:
bpftool btf dump file <path to kernel image with .BTF section> format c

cool, will check on that

to compilable .h file.

As for the description of this change, I'm wondering why all those steps to dump .BTF section into separate file is needed? Why can't you just point to kernel image w/ .BTF and use libbpf to extract BTF data using existing API. No hassle and it should just work.

there's not always kernel-debuginfo installed on the server and we plan
to keep the BTF info in the kernel rpm itself, so it's always there

but I'll change the code to check for vmlinux if it's available

thanks,
jirka

ajor

Partial review for files other than btf.cpp

ajor · 2019-07-17T04:56:47Z

cmake/FindLibBpf.cmake

+#  LIBBPF_LIBRARIES - Link these to use libbpf
+#  LIBBPF_DEFINITIONS - Compiler switches required for using libbpf
+
+#if (LIBBPF_LIBRARIES AND LIBBPF_INCLUDE_DIRS)


I don't claim to know what these lines are for in FindLibElf.cmake, but why are they commented out here?

Maybe it's so people can pass in paths via cmake -D stuff. If we aren't gonna use it here we should delete it

ajor · 2019-07-17T05:07:03Z

tests/CMakeLists.txt

@@ -27,6 +27,7 @@ add_executable(bpftrace_test
  ${CMAKE_SOURCE_DIR}/src/tracepoint_format_parser.cpp
  ${CMAKE_SOURCE_DIR}/src/types.cpp
  ${CMAKE_SOURCE_DIR}/src/utils.cpp
+  ${CMAKE_SOURCE_DIR}/src/btf.cpp


Keep these in alphabetical order please

ajor · 2019-07-17T05:10:50Z

src/CMakeLists.txt

@@ -41,6 +42,12 @@ else()
 endif()
 target_link_libraries(bpftrace ${LIBELF_LIBRARIES})

+if (LIBBPF_FOUND AND NOT STATIC_LINKING)


Is there no static library version of libbpf? I'm not keen on the static bpftrace having fewer features than the dynamic one

there is static version, I'll add it

ajor · 2019-07-17T05:14:56Z

src/ast/semantic_analyser.cpp

+     * If we are using curtak in field access, retype it
+     * to its original type: struct task_truct.
+     */
+    if (builtin.ident == "curtask" && builtin.is_field_access) {


Would people ever not want curtask defined as a struct like this? I'd say always treat it as a struct task_struct * and get rid of is_field_access

ok, will check

ajor · 2019-07-17T05:18:35Z

tests/CMakeLists.txt

@@ -43,6 +44,15 @@ if(HAVE_GET_CURRENT_CGROUP_ID)
 endif(HAVE_GET_CURRENT_CGROUP_ID)
 target_link_libraries(bpftrace_test arch ast parser resources)

+find_library(BPF_LIB bpf)


Isn't this covered by find_package(LibBpf) in the base CMakeLists.txt?

oops.. right, I'll remove it

danobi · 2019-07-19T17:06:59Z

It looks like libbpf in net-next has support for parsing BTF ( https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/tools/bpf/bpftool/btf.c#n349 ). IMO (and also spoke w/ @anakryiko), it'd be better if we reused that code instead of writing our own parser.

It looks like your PR directly constructs bpftrace struct info but I wonder if it'd be cleaner if we had libbpf dump the type info as a C header into a string buffer and then gave LLVM/clang the string buffer.

anakryiko · 2019-07-19T17:35:00Z

One interesting guarantee for btf_dump__dump_type() from libbpf, which I'm not sure I emphasized anywhere, is that it will emit only requested types and all the types to ensure correct compilation, skipping everything else from BTF that's not referenced. So for bpftrace, if you already know which types are referenced from user program, then using btf_dump__dump_type() selectively will allow to produce minimal set of type definitions to satisfy build. This should improve debuggability, if anything goes wrong (less types to weed through), as well as generally speed up compilation, I'd imagine.

olsajiri · 2019-07-22T17:17:23Z

cool, did not know about this.. will check, and repost, thanks

olsajiri · 2019-07-29T19:27:55Z

Pushed out new version with following changes:

using btf_dump to get type C code and passing it through clang compile interface to get type information as suggested earlier
supporting so far only vmlinux BTF extraction, not to confuse things
add --btf option to force BTF processing, otherwise it's processed only if there are no type/includes specified

anakryiko · 2019-07-29T21:32:57Z

src/clang_parser.cpp

+  if (!btf.has_data())
+    return true;
+
+  // We need to process every type separately, because BTF::c_def


hm... from just briefly skimming and reading this comment, I think this is suboptimal.

It would be way more efficient to do btf_dump__new() once, then call btf_dump__dump_type() for each type you need, then compile resulting header file with all the definitions (it won't have duplicates). Creating and discarding btf_dump object for every single instance of referenced type is quite wasteful.

Also, keep in mind that btf__find_by_name() does a linear search across all types, so it might be more efficient to first create a set of type names you care about, then do single linear pass over all BTF types and dump those that match one of the name in a set. That way it's O(N) or O(NlogM) at most (if using sorted set) vs O(NM), where N is total number of BTF types, and M is number of types referenced from BPF program code.

ok, will change

anakryiko · 2019-07-29T21:35:12Z

src/btf.cpp

+  }
+
+  btf = btf__new(data, (__u32) size);
+  if (IS_ERR(btf))


the way it's supposed to be done with libbpf without (re-)defining your own IS_ERR/PTR_ERR, etc is to use libbpf_get_error() function. Like this:

int err; btf = btf__new(...); err = libbpf_get_error(btf); if (err) { // error handling }

Then you can just drop MAX_ERRNO, IS_ERR, PTR_ERR, IS_ERR_VALUE definitions.

ok, will change, thanks

new version pushed with review comments changes

anakryiko

I've reviewed only btf_dump parts, haven't looked at anything else

anakryiko · 2019-08-05T05:48:09Z

src/btf.cpp

+  int err = libbpf_get_error(btf);
+  if (err)
+  {
+    libbpf_strerror(err, err_buf, sizeof(err_buf));


reset btf to nullptr, otherwise destructor will attempt to free invalid pointer

right, fixed

anakryiko · 2019-08-05T05:51:30Z

src/btf.cpp

+  std::set<std::string> myset(set);
+  __s32 id, max = (__s32) btf__get_nr_types(btf);
+
+  for (id = 1; id < max && myset.size(); id++)


ugh.. should have checked bpf sources.. strange, fixed

anakryiko · 2019-08-05T05:52:05Z

src/btf.cpp

+  {
+    const struct btf_type *t = btf__type_by_id(btf, id);
+
+    if (t == NULL)


this should never happen

anakryiko · 2019-08-05T05:55:01Z

src/btf.cpp

+      return;
+    }
+
+    asprintf(&path, "/lib/modules/%s/build/vmlinux", uts.release);


FYI, I'm working on a patch for the kernel that will expose kernel BTF through procfs or sysfs, so it will be a stable path, hopefully making this searching of kernel BTF more reliable.

cool, also it'll be available without debuginfo package,
could you please CC me on that kernel change, thanks

sure, will do

anakryiko

Looks good to me from libbpf/BTF standpoint. I'm not a big fan of using global set for collecting resolved types, but I'll let people more familiar with bpftrace to comment on that.

Thanks!

anakryiko · 2019-08-05T17:38:53Z

src/btf.cpp

+      return;
+    }
+
+    asprintf(&path, "/lib/modules/%s/build/vmlinux", uts.release);


sure, will do

olsajiri · 2019-08-12T07:37:54Z

@ajor @mmarchini any comments on this one? thanks

olsajiri · 2019-08-21T06:52:43Z

ping ;-) @ajor @mmarchini @brendangregg

thanks

olsajiri · 2019-09-02T14:45:01Z

FYI I'll push new version with support to check and read /sys/kernel/btf/vmlinux

danobi

Just some small comments but overall I really like how simple this is.

Didn't look too close at the libbpf stuff. Assuming @anakryiko took a good look at it.

danobi · 2019-09-11T11:33:27Z

src/ast/ast.cpp

@@ -5,6 +5,8 @@
 namespace bpftrace {
 namespace ast {

+std::set<std::string> Expression::resolve = std::set<std::string>();


This can lead to SIOF issues if another static method / initializer uses this symbol. Instead, it's much safer to do a meyer's singleton. ie

ast.h

class Expression { ... static std::set<std::string>& getResolve(); ... };

ast.cpp

std::set<std::string>& Expression::getResolve() { static std::set<std::string> s; return s; }

ok, will change

danobi · 2019-09-11T11:52:47Z

src/ast/semantic_analyser.cpp

+    /*
+     * Retype curtask to its original type: struct task_truct.
+     */
+    if (builtin.ident == "curtask") {


Probably split this out into its own else if branch. It looks like curtask is difference enough from the other things in this branch now.

danobi · 2019-09-11T11:54:41Z

src/clang_parser.h

@@ -30,6 +33,8 @@ class ClangParser
                            unsigned num_unsaved_files,
                            unsigned options);

+    bool check_diagnostics(std::string& input);


Doesn't look like input is being modified

Suggested change

bool check_diagnostics(std::string& input);

bool check_diagnostics(const std::string& input);

right, will use const in here

danobi · 2019-09-11T11:57:31Z

tests/btf_data.h

@@ -0,0 +1,48 @@
+#pragma once
+
+#if 0


Why not a comment instead?

habit ;-) I'll change for /* ... */

danobi · 2019-09-11T11:59:42Z

tests/btf_data.h

+  0x6f, 0x32, 0x00, 0x69, 0x6e, 0x74, 0x00, 0x6c, 0x6f, 0x6e, 0x67, 0x20,
+  0x69, 0x6e, 0x74, 0x00
+};
+unsigned int btf_data_len = 268;


This would be better so you don't have to count the number of elements

Suggested change

unsigned int btf_data_len = 268;

unsigned int btf_data_len = sizeof(btf_data) / sizeof(btf_data[0]);

danobi · 2019-09-11T12:37:31Z

src/btf.cpp

+  return vfprintf(stderr, msg, ap);
+}
+
+struct btf_location {


Can you put this at the top of this file so it's easier to spot?

Also, can you document what the raw field means? Not very clear from reading the struct def

danobi · 2019-09-11T13:41:36Z

src/btf.cpp

+  struct stat st;
+
+  if (stat(file, &st))
+    return NULL;


And also for the rest of this file

Suggested change

return NULL;

return nullptr;

danobi · 2019-09-11T13:46:08Z

src/btf.cpp

+    const struct btf_type *t = btf__type_by_id(btf, id);
+    const char *str = btf_str(btf, t->name_off);
+
+    std::set<std::string>::iterator it = myset.find(str);


Suggested change

std::set<std::string>::iterator it = myset.find(str);

auto it = myset.find(str);

danobi · 2019-09-11T13:48:06Z

src/ast/ast.h

@@ -5,6 +5,7 @@
 #include <map>
 #include <string>
 #include <vector>
+#include <set>


Prefer unordered_set. It's faster and I don't think we need the ordering, right?

Suggested change

#include <set>

#include <unordered_set>

right, no order is needed here

danobi · 2019-09-11T13:53:40Z

src/btf.cpp

+{
+  // 'borrowed' from libbpf's bpf_core_find_kernel_btf
+  // from Andrii Nakryiko
+  struct btf_location locs_normal[] =


Maybe use std::array instead. It knows its own size so you can do a rangeed for loop over it

ok, will check

on the other hand I need to know size of the std::array when
defining it, so it seems more convenient to me to use C array
in here.. however if you want to stick with c++ objects I understand
and can change it, let me know

Yeah that's kinda annoying. I think they're trying to add support for it in newer c++: https://en.cppreference.com/w/cpp/experimental/make_array . We can use that then

danobi

Whoops marked as approved by accident

olsajiri · 2019-09-13T13:52:00Z

thanks for review, I'll push new version

Adding default cast for curtask builtin expressions, so we don't need to cast it explicitly like in following script: # cat pid.bt #include <linux/sched.h> kprobe:do_dentry_open { printf("pid %d\n", curtask->pid); } Before: # bpftrace pid.bt Can not access field 'pid' on expression of type 'integer' printf: %d specifier expects a value of type integer (none supplied) After: # bpftrace pid.bt Attaching 1 probe... pid 4109 ...

Adding Expression::resolve string unordered set to keep a list of strings that need to be resolved. It will be used in following patches as a list to resolve with BTF data. It's static set so it's cleared in the Driver constructor, which is the starting point for parsing.

It will be detected on usual place, pluys user can specify non standard libbpf place (like /opt/libbpf/ below) with: $ cmake ... -DLIBBPF_INCLUDE_DIRS=/opt/libbpf/include \ -DLIBBPF_LIBRARIES=/opt/libbpf/lib64/libbpf.so Adding detection of btf_dump__new symbol, which is needed in following changes. If found the LIBBPF_BTF_DUMP_FOUND variable is set to TRUE.

Adding BTF class with constructor that checks on available BTF data in /sys/kernel/btf/vmlinux plus othe standard locations for vmlinux debuginfo. The list is 'borrowed' from libbpf's bpf_core_find_kernel_btf code from Andrii Nakryiko. If BTF data is loaded the BTF::has_data function returns true. Also linking bpftrace with libbpf.

Adding c_def function to BTF class that dumps C code definitions for given types set.

Adding ClangParser::visit_children function to contains clang_visitChildren call, so it can be used from multiple places.

Adding ClangParserHandler::check_diagnostics function to contain diagnostic messages processing, so it can be used from multiple places.

Adding SizedType::operator!= function, to ease up next changes.

Adding ClangParser::parse_btf_definitions function to load BTF definitions before parsing the user specified includes. Adding also check and warning in case those would collide.

Currently we always load BTF data before parsing user specified includes. Assuming that using include definitions and BTF data are mostly mutually exclusive, this change limits the BTF data processing to following cases: - there are no includes specified by user, or - the --btf option is specified

Adding BTF test to clang_parser modules with generated BTF data.

olsajiri · 2019-09-26T11:22:50Z

@danobi could you please check on the latest version? thanks

danobi

LGTM. Thanks! This is a great addition to bpftrace

mmarchini added the version-freeze label Jun 7, 2019

mmarchini removed version-freeze labels Jul 9, 2019

ajor reviewed Jul 17, 2019

View reviewed changes

olsajiri force-pushed the btf branch from 0a1bf92 to 1d46b50 Compare July 29, 2019 19:23

anakryiko reviewed Jul 29, 2019

View reviewed changes

olsajiri force-pushed the btf branch 2 times, most recently from e46f2a9 to 5b1ef77 Compare August 1, 2019 12:45

anakryiko suggested changes Aug 5, 2019

View reviewed changes

olsajiri force-pushed the btf branch from 5b1ef77 to 7304d75 Compare August 5, 2019 07:34

anakryiko reviewed Aug 5, 2019

View reviewed changes

src/btf.cpp Outdated

return;

}

asprintf(&path, "/lib/modules/%s/build/vmlinux", uts.release);

Copy link

anakryiko Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will do

olsajiri force-pushed the btf branch from 7304d75 to 56068cd Compare September 4, 2019 14:26

danobi approved these changes Sep 11, 2019

View reviewed changes

danobi requested changes Sep 11, 2019

View reviewed changes

olsajiri added 4 commits September 23, 2019 15:17

olsajiri force-pushed the btf branch from 56068cd to 15a6072 Compare September 23, 2019 13:17

olsajiri added 7 commits September 23, 2019 15:44

Add BTF::c_def function

2fdc83e

Adding c_def function to BTF class that dumps C code definitions for given types set.

Add ClangParser::visit_children function

f68c58f

Adding ClangParser::visit_children function to contains clang_visitChildren call, so it can be used from multiple places.

Add ClangParserHandler::check_diagnostics function

b8251fc

Adding ClangParserHandler::check_diagnostics function to contain diagnostic messages processing, so it can be used from multiple places.

Add SizedType::operator!= function

450a01b

Adding SizedType::operator!= function, to ease up next changes.

Add ClangParser::parse_btf_definitions function

cbf7da2

Adding ClangParser::parse_btf_definitions function to load BTF definitions before parsing the user specified includes. Adding also check and warning in case those would collide.

Add BTF tests

c665902

Adding BTF test to clang_parser modules with generated BTF data.

olsajiri force-pushed the btf branch from 15a6072 to c665902 Compare September 23, 2019 13:44

danobi approved these changes Sep 26, 2019

View reviewed changes

danobi merged commit 47621bb into bpftrace:master Sep 26, 2019

fbs mentioned this pull request Nov 7, 2019

feature: support to get type information from btf #383

Closed

olsajiri deleted the btf branch November 11, 2019 21:46

	bool check_diagnostics(std::string& input);
	bool check_diagnostics(const std::string& input);

	unsigned int btf_data_len = 268;
	unsigned int btf_data_len = sizeof(btf_data) / sizeof(btf_data[0]);

	std::set<std::string>::iterator it = myset.find(str);
	auto it = myset.find(str);

BTF kernel code support #734

BTF kernel code support #734

Conversation

olsajiri commented Jun 7, 2019

danobi commented Jun 26, 2019

anakryiko commented Jun 26, 2019

olsajiri commented Jun 26, 2019 • edited

ajor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danobi commented Jul 19, 2019

anakryiko commented Jul 19, 2019

olsajiri commented Jul 22, 2019

olsajiri commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakryiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakryiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olsajiri commented Aug 12, 2019

olsajiri commented Aug 21, 2019

olsajiri commented Sep 2, 2019

danobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danobi left a comment

Choose a reason for hiding this comment

olsajiri commented Sep 13, 2019

olsajiri commented Sep 26, 2019

danobi left a comment

Choose a reason for hiding this comment

olsajiri commented Jun 26, 2019 •

edited