bcc internals: Keep track of BPF progs using function instead of section

[davemarchevsky]

Context: I'm working on a project to enable bcc to create object files which can be loaded by libbpf's loader. This will make enabling "modern" libbpf features - e.g. global variables - significantly easier and by its nature would make an AOT-compiled mode for bcc straightforward. bcc programs should be able to use this mode with minimal code changes.

In order to do this we need to support libbpf-style SEC attribute annotations, which requires some modification of internal loader/bookkeeping plumbing. The summary of the major commit in this branch elaborates:

bcc currently identifies each individual BPF prog in an object file by
putting the prog in a special ".bpf.fn.$FUNC_NAME" section when
preprocessing the AST. After JITting an object file, the location and
size of the section are considered to be "the function's insns".

In order to support libbpf-style loading, we need to support its
sec_def-style SEC() attributes e.g. SEC("tp_btf/softirq_entry"), which
allow libbpf to determine the type of BPF prog - and often other
information like where to attach - based on the section name. These are
not guaranteed to be unique per function, so we can no longer assume
that a section contains only one function.

This commit gets rid of that assumption. bcc now finds the symbol in the
JITed image matching each BPF prog function and uses it to determine
size/location.

Also, this commit only adds the ".bpf.fn.$FUNC_NAME" section attribute
iff there isn't already a custom section attribute set.

Status: I need to address a few things before this is in a mergeable state

• Does BPFModule still need sections_? If not, let's get rid of it
• Need to clean up prog_func_info_-owned memory similarly to sections_ now (when not using rw_engine)
• Custom section names should not break execution, but they do currently
• debug dump (SOURCE_DEBUG) should work w/ custom sec names
  • Sanity check rest of debug flags too
• Figure out correct LLVM version checks
• Test on all python tools and some cpp applications

[davemarchevsky]

[buildbot, test this please]

[davemarchevsky]

[buildbot, ok to test]

[davemarchevsky]

section_ can also be set when object is loaded (and is - see notifyObjectLoaded)

I'm inclined to remove this in favor of notifyObjectLoaded as that's at the end of the compilation process and therefore leaves less (no?) room for unexpected section changes that the FrontendAction might not know about.

[yonghong-song]

There are quite some test failures. Maybe trying to resolve them first? For example, the following is a link to fc27 failure:
https://buildbot.iovisor.org/jenkins/job/bcc-pr/1183/label=fc27/console

You can run the test on your local machine easily for most tests.
For example, based on cmake file,
add_test(NAME py_test_tools_smoke WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
COMMAND ${TEST_WRAPPER} py_test_tools_smoke sudo ${CMAKE_CURRENT_SOURCE_DIR}/test_tools_smoke.py)
you can do sudo ./test_tools_smoke.py to run this test.

[davemarchevsky]

@yonghong-song this is finally in a reasonable state now that test infra is better, please take a look when you have the chance

edit: But I'd like to do the last step of manual testing (in checklist) before it gets merged

[yonghong-song]

Thanks @davemarchevsky I will look at the patch tomorrow.

[yonghong-song]

I tried the patch with latest upstream llvm, which I built clang/llvm binary from source. I got

$ sudo python3.6 syscount.py
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).
Aborted

The same for

$ sudo python3.6 profile.py
Sampling at 49 Hertz of all threads by user + kernel stack... Hit Ctrl-C to end.
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).
Aborted

Could you take a look?

[yonghong-song]

Okay, I did a little bit study on my above issues. It happened with latest upstream llvm. The reason is due to function signature change. The following can fix the compilation issue.

diff --git a/src/cc/bpf_module.cc b/src/cc/bpf_module.cc
index 50e1dc34..8ba77cfd 100644
--- a/src/cc/bpf_module.cc
+++ b/src/cc/bpf_module.cc
@@ -88,6 +88,45 @@ class MyMemoryManager : public SectionMemoryManager {
     return Addr;
   }
 
+// FIXME: did not check earlier llvm versions.
+#if LLVM_MAJOR_VERSION >= 14
+  void notifyObjectLoaded(ExecutionEngine *EE,
+                          const object::ObjectFile &o) override {
+    for (const std::pair<object::SymbolRef, uint64_t> &P : object::computeSymbolSizes(o)) {
+      object::SymbolRef Sym = P.first;
+      std::string SourceFileName;
+
+      Expected<object::SymbolRef::Type> SymTypeOrErr = Sym.getType();
+      if (!SymTypeOrErr)
+        continue;
+
+      object::SymbolRef::Type SymType = *SymTypeOrErr;
+      if (SymType != object::SymbolRef::ST_Function)
+        continue;
+
+      Expected<StringRef> NameOrErr = Sym.getName();
+      if (!NameOrErr)
+        continue;
+
+      Expected<uint64_t> AddrOrErr = Sym.getAddress();
+      if (!AddrOrErr)
+        continue;
+
+      StringRef Name = *NameOrErr;
+      auto info = prog_func_info_->get_func(Name.str());
+      if (info) {
+        auto SecIterOrErr = Sym.getSection();
+        if (!SecIterOrErr)
+          continue;
+        auto SecIter = *SecIterOrErr;
+        auto SecNameOrErr = (*SecIter).getName();
+        if (!SecNameOrErr)
+          continue;
+        info->section_ = (*SecNameOrErr).str();
+        info->size_ = P.second;
+      }
+    }
+  }
+#else
   void notifyObjectLoaded(ExecutionEngine *EE,
                           const object::ObjectFile &o) override {
     auto sizes = llvm::object::computeSymbolSizes(o);
@@ -106,6 +145,7 @@ class MyMemoryManager : public SectionMemoryManager {
       }
     }
   }
+#endif
 
   sec_map_def *sections_;
   ProgFuncInfo *prog_func_info_;

I briefly looked at the code. Look good to me in high level. I will go through another round in detail once the patch is updated to fix the issues with llvm14.

[davemarchevsky]

@yonghong-song nice catch!

I haven't been able to repro yet, have been struggling to get LLVM to build in a docker container in Debug mode. Will hack on that for a bit more, but if I don't reach a good state in a few hours will just use your fix.

Speaking of which, I looked in the code and it seems that this Expected<T> must be checked before access or destruction. fatal error will only happen if LLVM is built in Debug mode, or more precisely if LLVM_ABI_BREAKING_CHECKS is set (see assertIsChecked() and LLVM_ENABLE_ABI_BREAKING_CHECKS ifdefs in Expected). Can you confirm that your build was with one of these?

[yonghong-song]

@davemarchevsky the following is my llvm cmake build command,

cmake .. -DCMAKE_BUILD_TYPE=Release -G Ninja \
    -DLLVM_ENABLE_PROJECTS="clang;lld" \
    -DLLVM_TARGETS_TO_BUILD="BPF;X86" \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_ENABLE_ZLIB=OFF \
    -DCMAKE_INSTALL_PREFIX=$PWD/install

In the auto generated code, I do have LLVM_ENABLE_ABI_BREAKING_CHECKS equal to 1.

include/llvm/Config/abi-breaking.h:#define LLVM_ENABLE_ABI_BREAKING_CHECKS 1

[yonghong-song]

Also, for your information, my code is essentially copied from/similar to below
https://github.com/llvm/llvm-project/blob/main/llvm/lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp#L244-L285

[davemarchevsky]

@yonghong-song Updated and revalidated - but I was never able to repro the exact errors you were seeing in a docker environment. I went with a smaller change to deal with Expected than the one you suggested:

--- a/src/cc/bpf_module.cc
+++ b/src/cc/bpf_module.cc
@@ -92,18 +92,25 @@ class MyMemoryManager : public SectionMemoryManager {
                           const object::ObjectFile &o) override {
     auto sizes = llvm::object::computeSymbolSizes(o);
     for (auto ss : sizes) {
-      std::string name = ss.first.getName()->str();
+      auto maybe_name = ss.first.getName();
+      if (!maybe_name)
+        continue;
+
+      std::string name = maybe_name->str();
       auto info = prog_func_info_->get_func(name);
-      if (info) {
-        StringRef sec;
-#if LLVM_MAJOR_VERSION >= 10
-        sec = *ss.first.getSection().get()->getName();
-#else
-        ss.first.getSection().get()->getName(sec);
-#endif
-        info->section_ = sec.str();
-        info->size_ = ss.second;
-      }
+      if (!info)
+        continue;
+
+      auto section = ss.first.getSection();
+      if (!section)
+        continue;
+
+      auto sec_name = section.get()->getName();
+      if (!sec_name)
+        continue;
+
+      info->section_ = sec_name->str();
+      info->size_ = ss.second;
     }
   }

(LLVM_MAJOR_VERSION check removed because this doesn't work on <11 anyways, so merging this implicitly bumps min required version to 11, which is fine per #3808)

[yonghong-song]

tested with llvm13/llvm14 and in-progress llvm15. all passed. did another round of source checking, looks good to me.