refactor(libsinsp): export static fields via static methods in `threadinfo` and `fdinfo` #2296

ekoops · 2025-02-28T11:43:34Z

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

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

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

This PR makes define_static_field constexpr static and allows to directly the field offset. It introduces OFFSETOF_STATIC_FIELD, DEFINE_STATIC_FIELD_READONLY and DEFINE_STATIC_FIELD macros to hide the complexity behind extracting the field type and offset needed for define_static_field. This foundation is used to define two additional static methods an additional static method sinsp_threadinfo::get_static_fields() that can be used for the global initialization of s_threadinfo_static_fields in threadinfo.cpp. This is useful to avoid instantiating a sinsp_threadinfo object to initialize s_threadinfo_static_fields, a preliminary step needed before isolating the sinsp_thread_manager dependencies and passing them directly to its constructor. Moreover, to align the fdinfo implementation, this PR adds the static method sinsp_fdinfo::get_static_fields() to the fdinfo implementation.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

gnosek · 2025-02-28T14:26:09Z

/approve

github-actions · 2025-02-28T14:27:09Z

Perf diff from master - unit tests

     6.31%     -0.48%  [.] sinsp_parser::reset
     6.49%     -0.40%  [.] sinsp::next
     1.07%     -0.33%  [.] libsinsp::sinsp_suppress::process_event
     9.51%     +0.28%  [.] sinsp_thread_manager::create_thread_dependencies
     3.91%     +0.27%  [.] next_event_from_file
     0.97%     -0.26%  [.] sinsp_parser::event_cleanup
     1.40%     +0.22%  [.] is_conversion_needed
     0.69%     -0.20%  [.] sinsp_evt::get_direction
     2.00%     +0.18%  [.] sinsp_thread_manager::find_thread
     5.28%     -0.17%  [.] sinsp_evt::get_type

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.0227         +0.0226           147           151           147           151
BM_sinsp_split_median                                          +0.0247         +0.0246           148           151           148           151
BM_sinsp_split_stddev                                          +1.1701         +1.1685             1             3             1             3
BM_sinsp_split_cv                                              +1.1219         +1.1205             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0903         +0.0902            54            59            54            59
BM_sinsp_concatenate_paths_relative_path_median                +0.0805         +0.0804            54            58            54            58
BM_sinsp_concatenate_paths_relative_path_stddev                +1.6767         +1.6754             1             2             1             2
BM_sinsp_concatenate_paths_relative_path_cv                    +1.4549         +1.4540             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0080         -0.0080            25            25            25            25
BM_sinsp_concatenate_paths_empty_path_median                   -0.0082         -0.0083            25            25            25            25
BM_sinsp_concatenate_paths_empty_path_stddev                   +0.0053         +0.0053             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       +0.0133         +0.0135             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.1078         +0.1077            52            57            52            57
BM_sinsp_concatenate_paths_absolute_path_median                +0.1105         +0.1105            52            57            52            57
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.5346         -0.5351             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.5799         -0.5803             0             0             0             0

codecov · 2025-02-28T14:29:53Z

Codecov Report

Attention: Patch coverage is 96.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.16%. Comparing base (b242889) to head (48f0507).
Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
userspace/libsinsp/state/table.h	0.00%	2 Missing ⚠️
userspace/libsinsp/fdinfo.cpp	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
- Coverage   77.17%   77.16%   -0.02%     
==========================================
  Files         224      224              
  Lines       30133    30136       +3     
  Branches     4605     4605              
==========================================
- Hits        23256    23255       -1     
- Misses       6877     6881       +4

Flag	Coverage Δ
libsinsp	`77.16% <96.25%> (-0.02%)`	⬇️

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gnosek · 2025-03-03T03:07:43Z

@ekoops I took a look at how offsetof manages not to hit ubsan and it turns out that at least in my stdlib headers, it's a compiler builtin:

/* Offset of member MEMBER in a struct of type TYPE. */
#define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)

So I guess we'd have to explicitly pass the offset rather than a pointer inside a NULL-based struct. The downside is that offsetof doesn't have any type information. In my (unrelated) experiments, the best I came up with was a macro using decltype(T{}.FIELD) and offsetof(T, FIELD) but that doesn't help us a lot if the whole point of the exercise is to avoid a default constructor for sinsp_threadinfo :) Still, the default constructor would never actually be invoked -- maybe an unimplemented free function or static method that returns sinsp_threadinfo would be good enough for decltype?

ekoops · 2025-03-03T05:10:31Z

@ekoops I took a look at how offsetof manages not to hit ubsan and it turns out that at least in my stdlib headers, it's a compiler builtin:
/* Offset of member MEMBER in a struct of type TYPE. */
#define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
So I guess we'd have to explicitly pass the offset rather than a pointer inside a NULL-based struct. The downside is that offsetof doesn't have any type information. In my (unrelated) experiments, the best I came up with was a macro using decltype(T{}.FIELD) and offsetof(T, FIELD) but that doesn't help us a lot if the whole point of the exercise is to avoid a default constructor for sinsp_threadinfo :) Still, the default constructor would never actually be invoked -- maybe an unimplemented free function or static method that returns sinsp_threadinfo would be good enough for decltype?

Hi @gnosek , thank you for the help! I conducted similar experiments and approximately came out with the same conclusions. I'll still try to investigate the macro path, hoping there's a possible way to avoid creating and object. I'll also explore the other suggestion and update you

ekoops · 2025-03-10T13:16:38Z

I conducted some experiments and reached the following conclusions:

the define_static_field could be updated to accept an offset (notice: the template parameter T will only be used in the function body):

template<typename T>
constexpr static const field_info& define_static_field(field_infos& fields, size_t offset, const std::string& name, bool readonly = false)

using offsetof(sinsp_threadinfo, <field_name>) for offset extraction, or any variation starting from a pointer (e,g.: offsetof(std::remove_reference<decltype(*thisptr)>::type, <field_name>)) introduces a compilation warning like the following:

warning: ‘offsetof’ within non-standard-layout type ‘sinsp_threadinfo’ is conditionally-supported [-Winvalid-offsetof]

given the signature in point 1, we could use something like the following for field offset extraction (more on field type extraction later):

define_static_field<int64_t>(ret, reinterpret_cast<size_t>(&static_cast<sinsp_threadinfo*>(0)->m_tid), "tid");

Notice: I stil don't understand why ubsan complains with

auto ptr = static_cast<sinsp_threadinfo*>(0);
define_static_field<int64_t>(ret, reinterpret_cast<size_t>(&ptr->m_tid), "tid");

but doesn't complain for the aforementioned "inlined" approach...

Field type extraction could be done using an approach like

define_static_field<decltype(static_cast<sinsp_threadinfo*>(0)->m_tid)>(...)

We can introduce the following couple of macros to hide the aforementioned complexity:

#define DEFINE_STATIC_FIELD_READONLY(field_infos, container_type, container_field, name, readonly) \
	define_static_field<decltype(static_cast<container_type*>(0)->container_field)>(               \
	        field_infos,                                                                           \
	        reinterpret_cast<size_t>(&static_cast<container_type*>(0)->container_field),           \
	        name,                                                                                  \
	        readonly);
#define DEFINE_STATIC_FIELD(field_infos, container_type, container_field, name) \
	DEFINE_STATIC_FIELD_READONLY(field_infos, container_type, container_field, name, false)

Use the macros in point 5 like this:

DEFINE_STATIC_FIELD(ret, sinsp_threadinfo, m_pid, "tid");

Make `define_static_field` constexpr static and directly provide the field offset. Introduce `OFFSETOF_STATIC_FIELD`, `DEFINE_STATIC_FIELD_READONLY` and `DEFINE_STATIC_FIELD` macros to hide the complexicity behind extracting the field type and offset needed for `define_static_field`. Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>

FedeDP · 2025-03-12T14:19:41Z

/cc @jasondellaluce FYI

poiana · 2025-03-12T14:19:45Z

@FedeDP: GitHub didn't allow me to request PR reviews from the following users: FYI.

Note that only falcosecurity members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jasondellaluce FYI

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

FedeDP

/approve

poiana · 2025-03-13T09:00:41Z

LGTM label has been added.

Git tree hash: bc49dc98d5c1ced2d95d24d15ae52927df0f0849

Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>

poiana · 2025-03-17T09:57:55Z

LGTM label has been added.

Git tree hash: 0121213b94180658824f33b2a5c1346ce2c5c8c9

FedeDP

/approve

poiana · 2025-03-17T10:14:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ekoops, FedeDP, gnosek

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,gnosek]

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

github-project-automation bot added this to Falco Roadmap Feb 28, 2025

github-project-automation bot moved this to Todo in Falco Roadmap Feb 28, 2025

poiana added kind/cleanup release-note-none dco-signoff: yes area/libsinsp labels Feb 28, 2025

poiana requested review from Andreagit97 and hbrueckner February 28, 2025 11:43

poiana added the size/M label Feb 28, 2025

ekoops force-pushed the ekoops/static-fields branch 4 times, most recently from 07d4552 to 1cf9116 Compare February 28, 2025 12:00

poiana added size/L and removed size/M labels Feb 28, 2025

poiana added the approved label Feb 28, 2025

ekoops force-pushed the ekoops/static-fields branch from 1cf9116 to 782ed13 Compare February 28, 2025 15:29

ekoops force-pushed the ekoops/static-fields branch 2 times, most recently from 3ae89e9 to 1fdc4ef Compare March 11, 2025 14:37

poiana added size/XL and removed size/L labels Mar 11, 2025

ekoops force-pushed the ekoops/static-fields branch from 1fdc4ef to c9b4cd4 Compare March 11, 2025 17:00

poiana removed the size/XL label Mar 11, 2025

ekoops changed the title ~~refactor(libsinsp/threadinfo): export static fields via static method~~ refactor(libsinsp): export static fields via static methods in threadinfo and fdinfo Mar 12, 2025

FedeDP added this to the 0.21.0 milestone Mar 12, 2025

poiana requested a review from jasondellaluce March 12, 2025 14:19

FedeDP previously approved these changes Mar 13, 2025

View reviewed changes

poiana assigned FedeDP Mar 13, 2025

poiana added the lgtm label Mar 13, 2025

ekoops dismissed FedeDP’s stale review via 32a5bdc March 13, 2025 10:41

ekoops force-pushed the ekoops/static-fields branch from a716131 to 32a5bdc Compare March 13, 2025 10:41

poiana removed the lgtm label Mar 13, 2025

poiana requested a review from FedeDP March 13, 2025 10:41

ekoops added 2 commits March 13, 2025 14:03

refactor(libsinsp/threadinfo): export static fields via static method

d973eee

Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>

refactor(libsinsp/fdinfo): export static fields via static method

48f0507

Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>

ekoops force-pushed the ekoops/static-fields branch from 32a5bdc to 48f0507 Compare March 13, 2025 13:03

poiana added size/XL and removed size/L labels Mar 13, 2025

ekoops mentioned this pull request Mar 13, 2025

feat(userspace/libsinsp)!: remove sinsp::build_fdinfo() #2311

Merged

leogr approved these changes Mar 17, 2025

View reviewed changes

poiana assigned leogr Mar 17, 2025

poiana added the lgtm label Mar 17, 2025

FedeDP approved these changes Mar 17, 2025

View reviewed changes

github-project-automation bot moved this from Todo to In progress in Falco Roadmap Mar 17, 2025

poiana merged commit 15c4369 into falcosecurity:master Mar 17, 2025
47 checks passed

github-project-automation bot moved this from In progress to Done in Falco Roadmap Mar 17, 2025

ekoops deleted the ekoops/static-fields branch March 17, 2025 10:16

bevanjkay mentioned this pull request May 20, 2025

falcosecurity-libs 0.21.0 Homebrew/homebrew-core#224095

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(libsinsp): export static fields via static methods in `threadinfo` and `fdinfo` #2296

refactor(libsinsp): export static fields via static methods in `threadinfo` and `fdinfo` #2296

Uh oh!

ekoops commented Feb 28, 2025 •

edited

Loading

Uh oh!

gnosek commented Feb 28, 2025

Uh oh!

github-actions bot commented Feb 28, 2025 •

edited

Loading

Uh oh!

codecov bot commented Feb 28, 2025 •

edited

Loading

Uh oh!

gnosek commented Mar 3, 2025

Uh oh!

ekoops commented Mar 3, 2025

Uh oh!

ekoops commented Mar 10, 2025 •

edited

Loading

Uh oh!

FedeDP commented Mar 12, 2025

Uh oh!

poiana commented Mar 12, 2025

Uh oh!

FedeDP left a comment

Uh oh!

poiana commented Mar 13, 2025

Uh oh!

poiana commented Mar 17, 2025

Uh oh!

FedeDP left a comment

Uh oh!

poiana commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

refactor(libsinsp): export static fields via static methods in threadinfo and fdinfo #2296

refactor(libsinsp): export static fields via static methods in threadinfo and fdinfo #2296

Uh oh!

Conversation

ekoops commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnosek commented Feb 28, 2025

Uh oh!

github-actions bot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

Uh oh!

codecov bot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gnosek commented Mar 3, 2025

Uh oh!

ekoops commented Mar 3, 2025

Uh oh!

ekoops commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FedeDP commented Mar 12, 2025

Uh oh!

poiana commented Mar 12, 2025

Uh oh!

FedeDP left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Mar 13, 2025

Uh oh!

poiana commented Mar 17, 2025

Uh oh!

FedeDP left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

refactor(libsinsp): export static fields via static methods in `threadinfo` and `fdinfo` #2296

refactor(libsinsp): export static fields via static methods in `threadinfo` and `fdinfo` #2296

ekoops commented Feb 28, 2025 •

edited

Loading

github-actions bot commented Feb 28, 2025 •

edited

Loading

codecov bot commented Feb 28, 2025 •

edited

Loading

ekoops commented Mar 10, 2025 •

edited

Loading