new(libs): replace elfutils/libelf with elftoolchain/libelf (but with fork this time)

[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:

Following from discussion in #2174 this is an alternate implementation. Here, we are downloading elftoolchain/libelf from a forked repo which you can find here https://github.com/LucaGuerra/elftoolchain . As you can see, it only downloads libelf and it is also versioned. The content of the patches is the same.

Of course, if we like this idea and we end up merging it we'll work to port elftoolchain within the falcosecurity org instead of my own personal account :)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[github-actions]

Please double check driver/API_VERSION file. See versioning.

/hold

[github-actions]

Perf diff from master - unit tests

     6.72%     +1.46%  [.] sinsp_evt::get_type
    10.18%     -0.91%  [.] sinsp::next
     9.68%     +0.87%  [.] sinsp_parser::reset
     3.64%     -0.64%  [.] sinsp_evt::load_params
     2.81%     -0.63%  [.] is_conversion_needed
     5.83%     +0.50%  [.] next_event_from_file
     3.35%     -0.38%  [.] sinsp_thread_manager::get_thread_ref
     1.74%     -0.37%  [.] sinsp_evt_filter::sinsp_evt_filter
     1.58%     +0.33%  [.] scap_event_decode_params
     1.60%     +0.32%  [.] libsinsp::sinsp_suppress::process_event

Heap diff from master - unit tests

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

Heap diff from master - scap file

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

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.0329         -0.0329           151           146           151           146
BM_sinsp_split_median                                          -0.0332         -0.0332           150           145           150           145
BM_sinsp_split_stddev                                          -0.1703         -0.1701             2             1             2             1
BM_sinsp_split_cv                                              -0.1421         -0.1418             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0165         +0.0165            61            62            61            62
BM_sinsp_concatenate_paths_relative_path_median                +0.0170         +0.0170            61            62            61            62
BM_sinsp_concatenate_paths_relative_path_stddev                +0.4297         +0.4271             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    +0.4065         +0.4039             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0375         +0.0375            24            25            24            25
BM_sinsp_concatenate_paths_empty_path_median                   +0.0323         +0.0323            24            25            24            25
BM_sinsp_concatenate_paths_empty_path_stddev                   +4.2491         +4.2389             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       +4.0595         +4.0497             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0058         -0.0058            67            66            67            66
BM_sinsp_concatenate_paths_absolute_path_median                -0.0131         -0.0130            67            66            67            66
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.5820         -0.5820             1             0             1             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.5796         -0.5795             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0114         +0.0114           385           390           385           390
BM_sinsp_split_container_image_median                          +0.0139         +0.0139           385           390           385           390
BM_sinsp_split_container_image_stddev                          +0.2151         +0.2136             2             3             2             3
BM_sinsp_split_container_image_cv                              +0.2014         +0.1999             0             0             0             0

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.19%. Comparing base (5094053) to head (611c803).
Report is 47 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2175      +/-   ##
==========================================
+ Coverage   75.04%   75.19%   +0.15%     
==========================================
  Files         255      259       +4     
  Lines       33589    33875     +286     
  Branches     5739     5800      +61     
==========================================
+ Hits        25207    25473     +266     
- Misses       8382     8402      +20     

Flag	Coverage Δ
libsinsp	75.19% <ø> (+0.15%)	⬆️

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.

[LucaGuerra]

cc @Molter73 / @FedeDP (and @federico-sysdig ) do you like this solution? If so, I'll work to fix that pesky CI job so this will be ready for review. After merging I'll initiate the org transfer of the elftoolchain fork.

[Molter73]

cc @Molter73 / @FedeDP (and @federico-sysdig ) do you like this solution? If so, I'll work to fix that pesky CI job so this will be ready for review. After merging I'll initiate the org transfer of the elftoolchain fork.

My main concern now is that this approach will not work with hermetic builds (i.e no internet access), which we use for our project. I guess we could change the cmake script so we could make elftoolchain use the bundled or a non-bundled version like other dependencies, which in turn might allow adopters to keep using elfutils/libelf with some tinkering if they so choose (this last point would be a nice to have for us, but not a deal breaker).

[FedeDP]

I really like this approach!
To answer @Molter73 concerns, i think that in case one wants to link against system libelf, we can still link the same libelf we use today in our code: dynamic linking should not change much.
Instead, the new libelf should only provide the bundled libelf mode.
Even better, we could provide a

option(USE_BSD_LICENSED_LIBELF "Use https://github.com/LucaGuerra/elftoolchain instead of normal libelf" OFF)

In the end, the usage of such a library is only useful in Falco context, right?

EDIT: am not sure how much work would be needed to support both, ie: the old elfutils/libelf mode (the default IMHO), and the new one.

[Molter73]

To answer @Molter73 concerns, i think that in case one wants to link against system libelf, we can still link the same libelf we use today in our code: dynamic linking should not change much.

Yeah, I assumed something of the sort would work, just worried the new cmake module added will always attempt to download the new library. In any case, if it causes any issues in our workflows I'll fix it, so no need to worry 😄

[LucaGuerra]

Ok, now it works like this:

• USE_BUNDLED_LIBELF=ON -> downloads elftoolchain/libelf (BSD licensed) from our repos, builds and links it statically
• USE_BUNDLED_LIBELF=OFF -> uses whichever libelf is present in the system (most likely GNU elfutils), can be static or dynamic as before, dynamic linking works as before
• USE_BUNDLED_LIBBPF -> no change in behavior, you can download it or use the local one

[FedeDP]

I was wondering, isn't there a way to keep the exact same code as before, with just a change like:

       if (NOT USE_BSD_LIBELF)
          ExternalProject_Add(
		libelf
		PREFIX "${PROJECT_BINARY_DIR}/libelf-prefix"
		DEPENDS zlib
		URL "https://sourceware.org/elfutils/ftp/0.189/elfutils-0.189.tar.bz2"
		URL_HASH "SHA256=39bd8f1a338e2b7cd4abc3ff11a0eddc6e690f69578a57478d8179b4148708c8"
                ...
       else()
          ExternalProject_Add(
		libelf
		PREFIX "${PROJECT_BINARY_DIR}/libelf-prefix"
		DEPENDS zlib
		URL https://github.com/LucaGuerra/elftoolchain/releases/download/libelf-r4053-0/libelf-r4053-0.tar.gz
		URL_HASH SHA256=1861e6332d97f9cdc15a0c03b7b478b87abb47408ab41c50bebd9a384937e44e

?

By avoiding the FetchContent we have more control on the target name and we can keep the exact same variables as before and everything else should work as before.
Probably you already tried this one, but worth a shot.

[LucaGuerra]

There is an actual reason why we have FetchContent there: with FetchContent we are essentially plugging a repo (well, a zip file content) into our build tree; afterwards we are doing a CMake build with a native CMake target. Meaning: we are now using native target instead of variables. This allows us to stop caring about those variables and have the target already know include directories, object files and whatnot which in turn are propagated to dependencies. Specifically, libelf (elf) does not have dependencies, while libbpf (lbpf) depends on libelf (elf) and scap_engine_bpf also depends on libelf (elf) and libpman (pman) depends on libbpf. This way we don't need to tell cmake that, for instance, libpman depends on libelf because of a transitive dependency because it will already know this information.

This should pave the way for another patch I'm working on to essentially get rid of USE_BUNDLED_DEPS in favor of vcpkg.

[LucaGuerra]

@FedeDP : you see how we no longer need to tell libpman to go look for libelf? libpman does not use libelf directly. This is possible because we are building libelf as a native cmake target and "linking" it to the libbpf target

[FedeDP]

This should pave the way for another patch I'm working on to essentially get rid of USE_BUNDLED_DEPS in favor of vcpkg.

I see, that makes sense!

[LucaGuerra]

@Molter73 does this solution work for you? It should not change anything for your use case

[Molter73]

Ok, now it works like this:

* `USE_BUNDLED_LIBELF=ON` -> downloads elftoolchain/libelf (BSD licensed) from our repos, builds and links it statically

* `USE_BUNDLED_LIBELF=OFF` -> uses whichever libelf is present in the system (most likely GNU elfutils), can be static or dynamic as before, dynamic linking works as before

* `USE_BUNDLED_LIBBPF` -> no change in behavior, you can download it or use the local one

Awesome! Great work Luca!!

@Molter73 does this solution work for you? It should not change anything for your use case

Yup, this should require no changes on our side, thanks for the extra effort you put into this!

[LucaGuerra]

Thanks! If you like it, can you approve? I'll initiate the repo transfer and restore the Falco static build once merged

[Molter73]

/lgtm

[poiana]

LGTM label has been added.

Git tree hash: 1806f997a58f094601343e5ab284d419bfe6a8b8

[FedeDP]

/approve

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, LucaGuerra, Molter73

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 [FedeDP,LucaGuerra,Molter73]

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

[FedeDP]

/milestone 0.20.0

[LucaGuerra]

/unhold