wip: new(libs): replace elfutils/libelf with elftoolchain/libelf

[LucaGuerra]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area libscap-engine-modern-bpf

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

I would like to propose replacing elfutils/libelf with elftoolchain's libelf. The latter is an implementation of the libelf spec which is BSD licensed and therefore can be statically linked in Falco and its dependencies. The current situation has a few main disadvantages:

• Having a dynamically linked only libelf prevents us from having a static build which we used to have with musl
• It adds a dependency for our users, making the use of a binary package more complex and potentially failing
• It adds additional issues for clients of the Falco libs (not just Falco) because their build process is more complex in turn
• It may create additional issues if we try to switch to a different dependency management solution (e.g. vcpkg)

The proposed solution is based on Elftoolchain which I've adapted to work with libbpf and Falco libs.

So, how do we build this library? Upstream, the library is obtained via svn and built with BSD Make (which is different from GNU Make) and m4. Since none of those are currently dependencies for Falco or libs, I would propose NOT to require falco libs client to install those. So, I've generated the m4 files (hoping that they're not arch dependent, if they are I'll find a solution) and wrote CMake files just for libelf. In addition, some patches are in order (I don't know yet if they can be upstreamed but we may explore that). There is a README file that explains these steps in detail.

As you can see the PR is huge because it includes everything in the libs repo. We can discuss one of the following:

• leave it as-is (functional, but bloated repo)
• create another repo for our patched elftoolchain and download it from here (a bit more cumbersome, makes it harder to import patches because they'd require a release)
• remove everything from libs which is not required for libelf (less bloated repo, but would be harder to update)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [LucaGuerra]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Perf diff from master - unit tests

     5.66%     +1.40%  [.] sinsp::next
     5.21%     -0.99%  [.] sinsp_parser::process_event
     4.19%     +0.87%  [.] sinsp_evt::get_type
     0.95%     -0.69%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_emplace<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >
     8.61%     +0.57%  [.] sinsp_parser::reset
     1.08%     +0.57%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     1.73%     -0.53%  [.] libsinsp::sinsp_suppress::process_event
     1.17%     -0.49%  [.] scap_event_encode_params_v
     1.62%     -0.48%  [.] libsinsp::events::is_unknown_event
     1.25%     -0.48%  [.] sinsp_parser::parse_context_switch

Heap diff from master - unit tests

peak heap memory consumption: 64B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 72B

Heap diff from master - scap file

peak heap memory consumption: 64B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 64B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0486         +0.0486           144           151           144           151
BM_sinsp_split_median                                          +0.0554         +0.0555           144           152           144           152
BM_sinsp_split_stddev                                          -0.3741         -0.3740             2             1             2             1
BM_sinsp_split_cv                                              -0.4031         -0.4030             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0933         -0.0933            60            55            60            55
BM_sinsp_concatenate_paths_relative_path_median                -0.0933         -0.0933            60            55            60            55
BM_sinsp_concatenate_paths_relative_path_stddev                +0.2651         +0.2657             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    +0.3953         +0.3960             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0427         -0.0426            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_median                   -0.0415         -0.0415            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.6615         -0.6612             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.6464         -0.6462             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0931         -0.0931            61            56            61            56
BM_sinsp_concatenate_paths_absolute_path_median                -0.0899         -0.0899            61            56            61            56
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.4525         -0.4523             1             1             1             1
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.3963         -0.3961             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0309         +0.0309           381           393           381           393
BM_sinsp_split_container_image_median                          +0.0315         +0.0315           381           393           381           393
BM_sinsp_split_container_image_stddev                          -0.4916         -0.4917             4             2             4             2
BM_sinsp_split_container_image_cv                              -0.5069         -0.5070             0             0             0             0

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.04%. Comparing base (a339e9d) to head (a89b629).
Report is 40 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2174   +/-   ##
=======================================
  Coverage   75.04%   75.04%           
=======================================
  Files         255      255           
  Lines       33589    33589           
  Branches     5739     5739           
=======================================
  Hits        25207    25207           
  Misses       8382     8382           

Flag	Coverage Δ
libsinsp	75.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Molter73]

Hey @LucaGuerra! Thanks for putting this together, I just have some thoughts and questions I'd like to share with you.

My main concern is with maintenance of this new vendored dependency, while languages like Go and Rust have integrated tools to do this kind of thing (see go mod vendor and cargo vendor), C/C++ and/or CMake, do not. This means the dependency will need to be manually updated by following the steps described in the added README. While they are not terribly complicated, it is additional work that needs to be accounted for, but we might also be able to put some automation behind this process as well, so not terribly concerning. That is, until we get to the custom patches, this does seem like a bad idea, since updating the dependency will require re-applying the patches and that can be painful (speaking from experience here), if we can upstream the changes as you mentioned, all the better, but if we can't, a random commit on the main branch of the repo is really easy to miss/lose, so I would recommend keeping the patches on a separate diff file, or some other form of backup, just in case.

Another concern with this approach is how do we track what version of elfutils we are using? I haven't looked super deep into the PR, but I've not seen any mentions of a specific version being used. While this might not be terribly concerning now, it would be nice to have a way to know this in the future for multiple reasons (checking for CVEs, bugfixes, etc..).

Finally, for the proposed approaches:

• leave it as-is (functional, but bloated repo)

I don't like this one, that is all I have to say 🤣

• create another repo for our patched elftoolchain and download it from here (a bit more cumbersome, makes it harder to import patches because they'd require a release)

This one is my favorite. Correct me if I'm wrong, but since we are not planning to redistribute this library, the release could just be a tag on the new repo, right? We don't need to do anything fancier than that. Worst case, I guess we need to build the archive file for it and publish it as a github package?

• remove everything from libs which is not required for libelf (less bloated repo, but would be harder to update)

This is even more work to keep track of changes, I don't like it either.

Anyways, sorry about the mile long comment! 😅

[LucaGuerra]

Thank you for the insights @Molter73 ! Yes, maintaining a fork is everyone's favorite thing to do I know 🤣 And I do share each of your concerns. I didn't want to write a longer comment than I already did in the PR description so I'll answer the questions with the rationale

My main concern is with maintenance of this new vendored dependency

One thing that makes having a fork of this library feasible is that from looking at the history it is very stable and essentially implements an interface that has been there for a long time. The last commit to elftoolchain/libelf was in 2022, and I don't expect it to change drastically. This is also why I don't think automation is needed to maintain it for now. It is possible that we could upstream something from the patches but we need to check if those "extension" functions would be acceptable for upstream and write them keeping the style consistent with the rest of the project.

Another concern with this approach is how do we track what version of elfutils we are using?

The version number for it is just its released version (which however is a bit older) or the SVN revision number, the latter in this case.

It seems like everyone's favorite at this point is having a separate repo which is a fork with patches already applied. We can have a tiny "release" process that deals with the boring stuff such as generating the m4 files and puts together an importable source package, slaps a version tag and can be fetched from the main libs repo.

[LucaGuerra]

Please take a look at the alternate implementation with a fork: #2175

[LucaGuerra]

Closing in favor of #2175