Add systemtap's sys/sdt.h as depends for GUIX builds with USDT tracepoints (build system, utils/log/libs)

https://github.com/bitcoin/bitcoin/pull/23724

Host: 0xB10C  -  PR author: 0xB10C

The PR branch HEAD was 0705708 at the time of this review club meeting.

Notes

  • Userspace, Statically Defined Tracing (USDT) allows us to hook into tracepoints during Bitcoin Core runtime. Once a tracepoint is reached, it can pass data about process internals to a userspace script for further processing. This is great for observability and allows for debugging, testing, and monitoring. See “Userspace, Statically Defined Tracing support for Bitcoin Core” for more background information.

  • Initial build support for tracepoints was added in #19866. The tracepoints (e.g. TRACE1(context, name, arg0)) use (see src/util/trace.h) the DTRACE_PROBE macros (see systemtap:includes/sys/sdt.h#L486) from Systemtap under the hood. This requires the sys/sdt.h header to be present during compilation. If not present, ENABLE_TRACING will not be set to 1 in configure.ac, and no DTRACE_PROBE’s are used.

  • On Ubuntu/Debian, for example, the sys/sdt.h headers are included in the systemtap-sdt-dev package. If this package is installed, binaries with DTRACE_PROBE tracepoints will be built. Developers wanting to use the tracepoints can build the binaries themselves.

  • However, tracepoints should ideally be included in release builds. This allows, for example, hooking into the tracepoints of production deployments. No custom binaries (which might behave differently) need to be compiled and deployed to trace Bitcoin Core.

  • Tracepoints are NOPs in our Bitcoin Core binaries. As we make sure not to include any additional expensive computations solely for the tracepoints, there is only the minimal runtime overhead of the one NOP per tracepoint.

  • This PR adds the Systemtap package to Bitcoin Core’s depends system. GUIX builds for Linux platforms now contain tracepoints.

  • We have discussed User-Space, Statically Defined Tracing (USDT) in a previous review club.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. Did you manage to build a bitcoind binary including the tracepoints (see “Listing available tracepoints” in doc/tracing.md)? Did you do a GUIX build?

  3. For GUIX builds, why do we need to add the Systemtap sys/std.h header as a dependency instead of using the header file avaliable on the GUIX build host system?

  4. In which build step is ENABLE_TRACING set? Under which condition is it set? What happens when it’s set to 1?

  5. What do we need to have consensus on before this PR is merged?

  6. Why can we skip configure and make for the Systemtap dependecy? What are the problems and how are they solved?

  7. Can you verify that the tracepoints are NOPs in the binaries? If yes, how?

Meeting Log

  117:00 <b10c> #startmeeting
  217:00 <svav> Hi
  317:00 <michaelfolkson> hi
  417:00 <effexzi> Hi
  517:00 <b10c> feel free to say hi so we know you'r here! (lurking is also fine)
  617:00 <zonemix> hi
  717:01 <b10c> anyone joining the PR review club for the first time?
  817:01 <zonemix> yep this is my first time
  917:01 <b10c> welcome zonemix! feel free to ask questions anytime :)
 1017:01 <glozow> hi!
 1117:01 <ragu3> first timer here. hi:)
 1217:02 <glozow> zonemix: ragu3: welcome!
 1317:02 <b10c> welcome ragu3!
 1417:02 <b10c> today we are looking at https://github.com/bitcoin/bitcoin/pull/23724
 1517:02 <b10c> notes are here: https://bitcoincore.reviews/2372
 1617:03 <b10c> Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?
 1717:03 <svav> n but I read the notes
 1817:03 <michaelfolkson> Definite Concept ACK. "light conceptual agreement" undersells it :)
 1917:03 <azor> Hi everyone
 2017:04 <b10c> hi azor!
 2117:05 <michaelfolkson> Not sure whether this should be fixed first "The tracepoints are currently not automatically/CI tested" before this is merged (but I think that is a later question on Approach ACK/NACK)
 2217:05 <glozow> stickes-v: yes sorry, i should have posted an announcement about the PR change
 2317:05 <glozow> (re: convo from earlier)
 2417:05 <b10c> Did you manage to build a bitcoind binary including the tracepoints? Quick no/build/build-guix
 2517:06 <svav> no did not have time
 2617:06 <michaelfolkson> No, away from my Linux machine
 2717:06 <ccdle12> build-guix
 2817:06 <merkle_noob[m]> Hi everyone
 2917:07 <b10c> svav michaelfolkson: that's fine
 3017:07 <b10c> ccdle12: did you check if the binaries include the tracepoints?
 3117:08 <ccdle12> b10c: I used gdb to check the the list of probes?
 3217:08 <b10c> ccdle12: awesome!
 3317:08 <ccdle12> I ran log_p2p_traffic on signet and that worked, but the other didnt work for me, I assumed it was my machine :(
 3417:09 <b10c> ohh, which one didn't work?
 3517:09 <ccdle12> log_utxos was getting segfault
 3617:09 <ccdle12> and the python scripts that relied on bcc
 3717:10 <ccdle12> but I think thats because I probably didn't install some of the bcc dependencies correctly
 3817:11 <b10c> maybe, happy to debug that further later if you want. Saw some reports of people having issues with old bpftrace versions (e.g. the segfault)
 3917:11 <ccdle12> ahh ok sounds great, I havne't dug too deeply into it yet so didn't want to start giving false reports :)
 4017:12 <b10c> michaelfolkson: yes, that's a valid point. I've added "not tested by CI" on purpose to maybe only merge this PR once that's setup..
 4117:12 <b10c> next question: For GUIX builds, why do we need to add the Systemtap sys/std.h header as a dependency instead of using the header file avaliable on the GUIX build host system?
 4217:14 <michaelfolkson> Because it needs to be present during compilation?!
 4317:14 <michaelfolkson> Don't know
 4417:15 <svav> Because the header needs to be a special one that contains something that makes something get built
 4517:16 <b10c> We can't really assume that a user has the systemtap header or even the right version on the GUIX build host
 4617:16 <b10c> we want the builds to be deterministic for all GUIX builders, so can't rely on host system software
 4717:16 <glozow> gotta be d e t e r m i n i s t i c
 4817:16 <b10c> glozow: 💯
 4917:17 <b10c> GUIX builds are done in a GUIX container. this, for example, makes sure no host dependencies leak into the build
 5017:18 <b10c> now an autotools question: In which build step is ENABLE_TRACING set? Under which condition is it set? What happens when it’s set to 1?
 5117:19 <michaelfolkson> For the first timers Optech page on reproducible builds: https://bitcoinops.org/en/topics/reproducible-builds/
 5217:20 <svav> Re sys/std.h dependency, as you saying in a GUIX build, when you set it as a dependency, it will look for sys/std.h within the specific GUIX container?
 5317:21 <svav> *are you saying*
 5417:21 <glozow> ehhhh when making configure from configure.ac?
 5517:22 <b10c> svav: yes! we first build the depends and then copy them as _inputs_ for our Bitcoin Core build
 5617:22 <michaelfolkson> https://github.com/bitcoin/bitcoin/blob/42796742a45e3f12e82588afa77054c103fca05c/configure.ac#L1354
 5717:22 <b10c> sys/sdt.h is one of those depends
 5817:23 <b10c> (being added as one of those depends in this PR)
 5917:23 <b10c> michaelfolkson glozow: correct! what does this "AC_COMPILE_IFELSE" do?
 6017:24 <glozow> uhhhh i guess it tells autoconf something
 6117:24 <b10c> it includes the sys/sdt.h header and uses something that's named "DTRACE_PROBE", what could that be?
 6217:25 <glozow> https://github.com/bitcoin/bitcoin/blob/42796742a45e3f12e82588afa77054c103fca05c/configure.ac#L1349-L1357
 6317:25 <glozow> "hi autoconf, if sys/sdt.h is included and something, set ENABLE_TRACING=1 in configure script"
 6417:25 <ccdle12> compile the input below to check if we have the depdency for sdt
 6517:25 <merkle_noob[m]> From the PR, ENABLE_TRACING is set when sys/sdt.h is found and/or when use_usdt is set to yes.
 6617:25 <b10c> correct!
 6717:26 <b10c> we try to compile a small (2 loc) program during configure. that program includes sys/sdt.h and a simple tracepoint (the DTRACE_PROBE(context, event))
 6817:27 <michaelfolkson> Why include a simple tracepoint? To check if it is working?
 6917:28 <b10c> if this succeeds, we set ENABLE_TRACING to 1. We can we have the sys/sdt.h header and that it makes sense to it
 7017:28 <svav> DTrace provides a facility for user application developers to define customized probes in application code
 7117:30 <b10c> michaelfolkson: good question! there was a problem with a more primitive detection method of just checking for the header. see https://github.com/bitcoin/bitcoin/pull/22238
 7217:31 <b10c> svav: the DTrace naming here is confusing.. DTRACE_PROBE is in-fact an alias for a Systemtap probe for compability reasons (see sys/sdt.h)
 7317:32 <b10c> so when ENABLE_TRACING is set to 1, we define the TRACEx macros in https://github.com/bitcoin/bitcoin/blob/master/src/util/trace.h
 7417:32 <svav> b10c: You are saying DTrace and Systemtap are two different things?
 7517:33 <b10c> otherwise they are undefined and no tracepoints are included in the binary
 7617:34 <b10c> svav: yes. AFAIK DTrace is older and from Sun, Systemtap was developed specifically for Linux. There exists some interoperability between the too, but I haven't tested anything in this direction
 7717:35 <b10c> feel free to ask about the configure step. I'll continue with the next question in the meantime
 7817:35 <b10c> What do we need to have consensus on before this PR is merged?
 7917:35 <b10c> What should we test before this PR is merged?
 8017:37 <michaelfolkson> Well to state the obvious that the Systemtap header is sufficient to get tracepoints working
 8117:37 <svav> I don't run a Bitcoin node yet, but I'm thinking about it ... Presumably for all this Pull Request testing, people set up separate test environments that don't affect your main node, so what sort of technology is necessary for test environments? Thanks.
 8217:37 <michaelfolkson> Including the header shouldn't have any adverse impacts on non-users of tracepoints
 8317:38 <michaelfolkson> "Merging this now could leave us with broken tracepoints in a release build." <- You wouldn't want broken tracepoints
 8417:38 <b10c> michaelfolkson: right, probably also that we don't break and platforms (for whatever reasons, I don't expect that we break anything)
 8517:40 <b10c> also right. I think we should be able to get some consensus on including the tracepoints in release builds here
 8617:41 <michaelfolkson> svav: I think it varies widely. I have a Linux laptop and Mac laptop for dev stuff. But others will use VMs, VPS, Docker etc
 8717:41 <b10c> svav: yes I run a few nodes in test setting but also for development
 8817:41 <b10c> in a*
 8917:42 <svav> So, can you run multiple bitcoind on the same computer if you set some as test environments, or do you need to use virtual environments for each bitcoind?
 9017:43 <b10c> I also think we should have a bit more confidence that the tracepoints aren't broken by automatically testing them in the CI (or similar). michaelfolkson mentioned this earlier
 9117:44 <b10c> svav: yes, you can run multiple, even multiple on mainnet. You'll need to set different ports and data directories though
 9217:44 <b10c> The next question is a hard one:
 9317:44 <svav> b10c: ok thanks
 9417:44 <b10c> Why can we skip configure and make for the Systemtap dependecy? What are the problems and how are they solved?
 9517:45 <michaelfolkson> b10c: How do you test them in the CI? Add a tracepoint test?
 9617:47 <b10c> michaelfolkson: yes, e.g. in the Python test suite. It's a bit tricky as you need special permissions to hook into the tracepoints, but I think it's doable. See https://github.com/bitcoin/bitcoin/issues/23296
 9717:49 <b10c> We can (and want to) skip configure and make for systemtap as we only need the header file, we don't need the systemtap tool for our build
 9817:49 <michaelfolkson> Ok cool, so the plan is to add them in this PR or a separate PR? I guess that gets the Approach ACK for me
 9917:50 <b10c> We don't want to build stuff we don't end up using in our build. That just wastes resources
10017:50 <b10c> michaelfolkson: in a separate PR, that's not in-scope for GUIX builds
10117:51 <merkle_noob[m]> Exactly the answer I wanted to give :)
10217:51 <b10c> the problem with the sys/sdt.h header file is it includes a sdt-config.h which is only created in the configure step..
10317:52 <b10c> sdt-config.h defines if an assembly feature can be used.
10417:52 <b10c> This feature check was added in 2010 and we assume the feature can be used
10517:52 <b10c> We apply a patch to the sys/sdt.h file that allows us to use it without the configure step.
10617:53 <b10c> any questions regarding this?
10717:54 <merkle_noob[m]> b10c: Please, could that assumption be broken?
10817:54 <svav> Is an assembly feature referring to a specific assembly or the concept of an assembly?
10917:55 <merkle_noob[m]> Or under what circumstances could the assumption not hold?
11017:55 <b10c> yes, it can. I sadly haven't found any documentation on that feature being added to gcc (all documentation I saw just lists it as 'this is supported')
11117:56 <b10c> maybe on some very old GCC version or uncommon platform/architecture?
11217:56 <merkle_noob[m]> b10c: OK I see. Thanks.
11317:57 <b10c> svav: a feature of the byte code assembly, does this answer your question?
11417:57 <b10c> last question: Can you verify that the tracepoints are NOPs in the binaries? If yes, how?
11517:58 <svav> b10c: yes thanks
11617:59 <b10c> We can use `readelf -n bitcoind` or `info probe` in gdb to list the locations of the tracepoints in the binary.
11717:59 <b10c> and then, for example, gdb to show us the assembly (byte code) at the tracepoint location
11817:59 <svav> Does NOP stand for Non Op Code? Why is it important that a tracepoint is a NOP?
11918:00 <b10c> or `objdump -d` works too
12018:00 <b10c> I have notes for this here: https://gist.github.com/0xB10C/0edbbbe462fc70a0c298f64aa73ff37c
12118:00 <b10c> svav: yes, a NOP does _nothing_
12218:01 <michaelfolkson> no-op (no operation)
12318:01 <b10c> the tracing uses it to hook into that exact instruction
12418:01 <b10c> #endmeeting