tracing: macOS USDT support

kouloumos commented at 4:38 pm on July 4, 2022: contributor

This PR adds macOS support for User-Space, Statically Defined Tracing (USDT) as well as dtrace example scripts based on the existing bpftrace scripts for Linux. This is tested on macOS 10.15.

Overview

The current implementation of USDT only supports Linux as the DTRACE_PROBE*(context, event, ...args) macros are not supported on macOS. As initially referenced in #22238, the process of using USDT probes on macOS is slightly different and it’s described in the BUILDING CODE CONTAINING USDT PROBES section in the dtrace(1) manpage.

This more involved process includes

Creating the providers description file util/probes.d which defines the providers and specifies their probes.
Use util/probes.d to generate the header file util/probes.h with the neccesary tracepoints macros which are of the format CONTEXT_EVENT(...args).
Add the tracepoints macros to util/trace.h.

Notes

~Part of the macOS process is to create the providers description in a .d file. Therefore, types not supported by the D language (specifically bool and std::byte) cannot be used as part of the probe’s signature. On those occasions, in lack of a better solution, only supported types are used and then a patch is applied at the generated util/probes.h file to replace the temporary D-supported types with those that we actually need.~

~You can reproduce the initial header file by running dtrace -h -s src/util/probes.d -o src/util/probes.h (compiling with that results to errors because of not matching types) and then apply the probes-fix.diff patch with git apply probes-fix.diff.~ Edit: util/probes.h is now generated during build time after changes (see #25541 (review) and #25541 (review)) that removed the need for the non supported types.

Having the bitcoind binary compiled with USDT support, you can then use the dtrace example scripts connectblock_benchmark.d, log_p2p_traffic.d, log_utxos.d (root privileges needed) to test the macOS support. Extra documentation for those can be found at contrib/tracing as they are functionally the same as the existing bpftrace scripts.

Adding tracepoints to Bitcoin Core (extra steps after this PR)

After this PR when a new tracepoint is added, we will need to

Define the new probe for the tracepoint at util/probes.d together with a new provider if needed.
Add a new #define context_event CONTEXT_EVENT macro at util/trace.h. This might not be final as discussed at #25541 (review).

fanquake added the label macOS on Jul 4, 2022

michaelfolkson commented at 4:53 pm on July 4, 2022: contributor

Concept ACK. It would be great to support tracing on MacOS assuming the maintenance burden is satisfactory.

(The linter checks are failing currently.)

in src/util/probes.d:12 in b5148f53fb outdated

 7+   This file is part of the more involved process of using USDT probes on macOS
 8+   as described in the BUILDING CODE CONTAINING USDT PROBES section in the
 9+   dtrace(1) manpage https://www.unix.com/man-page/mojave/1/dtrace/
10+
11+   This defines the providers and specifies their probes in order to generate
12+   the header file `probes.h` with the neccesary tracepoints macros.

brunoerg commented at 5:03 pm on July 4, 2022:

nit:

0   the header file `probes.h` with the necessary tracepoints macros.

brunoerg commented at 5:04 pm on July 4, 2022: contributor

Concept ACK

in configure.ac:1407 in b5148f53fb outdated

1397+else
1398+  use_usdt=no
1399+fi
1400+
1401+if test x$use_usdt = xyes; then
1402+  AC_DEFINE([ENABLE_TRACING], [1], [Define to 1 to enable tracepoints for Userspace, Statically Defined Tracing])

hebasto commented at 5:13 pm on July 4, 2022:

If this PR is macOS-specific, why remove Linux-specific check for DTRACE_PROBE macro?

kouloumos commented at 5:49 pm on July 4, 2022:

Before #22238 AC_CHECK_HEADER([sys/sdt.h],...)was the check to detect whether USDT tracepoints are supported, but per the reasoning of the aformentioned PR, the indent of that change was to “fail” on macOS and not on Linux as macOS has the sys/sdt.h header but DTRACE_PROBE is not defined in it.

So even after this macOS-specific PR, if I did not change that, the binary would compiled with USDT tracing disabled as the DTRACE_PROBE is still not defined. That’s why I revert that change, as now there is a macOS specific clause in util/trace.h

kouloumos commented at 9:14 am on July 11, 2022:

For reference, my understanding of the effects of that PR was wrong, as it seems that having sys/sdt.h header but not defined DTRACE_PROBE is not a macOS-specific pattern. I have since reverted this change.

kouloumos force-pushed on Jul 4, 2022

0xB10C commented at 9:48 am on July 5, 2022: contributor

Concept ACK. Thank you for working on this :rocket: I plan to review this. I want to see e.g. what additional things we would need to consider when adding new tracepoints. A few first impressions, notes, and questions:

This looks like a cool PoC, but not really close to being ready for merge, especially with the custom patch :). Could also be a draft PR.
Awesome to see that we can re-use the same TRACEx macros we’ve already placed in the code. This was how I envisioned macOS (or Windows if possible) tracing support.

and then apply the probes-fix.diff patch with git apply probes-fix.diff.

Would be good find a workaround for this. Did you create the patch manually? Can we change the tracepoints to improve this? I.e. not use bool?
Does src/util/probes.h need to be in version control or can it be generated during build time?
I was able to generate src/util/probes.h from src/util/probes.d on Linux, with dtrace supplied by Systemtap. However, it looks very different from your probes.h file. See this gits.
It would be ideal if we could have a probes.d file that works for both Linux and macOS.
The *_ENABLED() in probes.h reminded me of https://eklitzke.org/how-sytemtap-userspace-probes-work
We should check the macOS GUIX builds at some point to see if tracing works with them.
We’d probably want to document these steps in doc/tracing.md once we’ve figured something out that works well and doesn’t add to much maintenance. Also, the *.d scripts can be mentioned under the example section.

cc @jb55

in src/util/probes.d:22 in 1aacde12c9 outdated

21+*/
22+
23+provider net {
24+        probe inbound_message(int64_t, char *, char *, char *, int64_t, unsigned char *);
25+        probe outbound_message(int64_t, char *, char *, char *, int64_t, /* std::byte * */unsigned char *);
26+};

0xB10C commented at 9:50 am on July 5, 2022:

does inbound_message also need the /* std::byte * */ comment? They both pass the same data.

kouloumos commented at 10:09 am on July 5, 2022:

I actually made a mistake with the comment there, it’s the other way around. inbound_message is the one that needs the change to std::byte *. outbound_message works as it is with unsigned char *.

Trying to build without changing the type at inbound_message results to

0error: no matching function
1...
2/util/probes.h:46:13: note: candidate function not viable: no known conversion from 'CDataStream::value_type *' (aka 'std::byte *') to 'const unsigned char *' for 6th argument

That’s why I changed that to std::byte *. Reading the docs, my understanding was also that the both pass the same data but it seems that the 6th arg of inbound_message uses the CDataStream class while outbound_message uses a std::vector<unsigned char>.

0xB10C commented at 10:33 am on July 5, 2022:

Oh, you are right. Then we should reinterpret_cast the .data() in inbound_message to const unsigned char * if it makes thinks easier for you (and is currently not conforming to the documentation).

kouloumos commented at 1:52 pm on July 5, 2022:

Great idea! (unsigned char*)(msg.m_recv.data()) seems to work. But I am not sure if this has performance overhead.

in src/util/trace.h:17 in 1aacde12c9 outdated

12+#include <util/macros.h>
13+#include <util/probes.h>
14+
15+// since the dtrace macros are automatically generated in uppercase, additional
16+// macros are needed to translate the lowercase context & event names into the
17+// required uppercase CONTEXT_EVENT macros

0xB10C commented at 9:53 am on July 5, 2022:

we could change that, if this makes it a lot easier for macOS. This would probably be an API break, but I think the tracepoints are still experimental enough to be able to do it.

kouloumos commented at 3:08 pm on July 5, 2022:

Changing to uppercase was part of my initial implementation but I changed my mind. Even if we need those uppercase macros for macOS, the actual tracepoints for macOS would still be of the format net:inbound_message. But my understanding is that if we change TRACEx to TRACE6(NET, INBOUND_MESSSAGE, ..args) the Linux tracepoints would be NET:INBOUND_MESSSAGE thus not having a unified format across systems.

bitcoin deleted a comment on Jul 5, 2022

laanwj added the label Scripts and tools on Jul 5, 2022

kouloumos commented at 2:57 pm on July 5, 2022: contributor

Thank you for the review @0xB10C! I am not happy with that custom patch either, but this being my first interaction with the build system, I was not sure what is consider acceptable. In retrospect, it is not and it adds unnecessary maintenance burden.

Would be good find a workaround for this. Did you create the patch manually? Can we change the tracepoints to improve this? I.e. not use bool?

If what you mentioned at #25541 (review) is acceptable and we find a way to not use bool for the tracepoints, the generation of the header file could possibly become part of the build process. I avoided thinking about changing the bool type, mostly because of the overhead that this might introduced.

Does src/util/probes.h need to be in version control or can it be generated during build time? …

The *_ENABLED() in probes.h reminded me https://eklitzke.org/how-sytemtap-userspace-probes-work

Looking into the probes_sdt branch in that article, it seems that generating it during build time might be possible. This would possibly result to a probes.d file that works for both Linux and macOS. I initially avoided going that route because of the currently required changes (patch) after the file generation.

I was able to generate src/util/probes.h from src/util/probes.d on Linux, with dtrace supplied by Systemtap. However, it looks very different from your probes.h file. See this gits.

I think being different makes sense as the resulting file has a different target os? My probes.h file is not the actual result of the command, as the committed file is after the manual changes shown in the patch. I’ve updated the patch link with the initial (before applying the patch) probes.h file.

We’d probably want to document these steps in doc/tracing.md once we’ve figured something out that works well and doesn’t add to much maintenance. Also, the *.d scripts can be mentioned under the example section.

Yes, I plan to enhance the docs as soon as this works well.

I’ll convert this to a draft PR until I find a way to get rid of the patch and in the meantime fix the failing checks. Best case scenario: replace bool > generate util/probes during build time > 🚀

kouloumos marked this as a draft on Jul 5, 2022

kouloumos force-pushed on Jul 6, 2022

in src/util/probes.d:32 in 64d0862e31 outdated

24+
25+provider utxocache {
26+        probe flush(int64_t, uint32_t, uint64_t, uint64_t, uint8_t);
27+        probe add(unsigned char *, uint32_t, uint32_t, int64_t, uint8_t);
28+        probe spent(unsigned char *, uint32_t, uint32_t, int64_t, uint8_t);
29+        probe uncache(unsigned char *, uint32_t, uint32_t, int64_t, uint8_t);

0xB10C commented at 7:38 pm on July 6, 2022:

I was going to recommend to try using uint8_t or similar here instead of the non-existing bool. I see you already did that. I think changing this is preferred over changing the actual tracepoint arguments. We might want to add a comment on this tough.

kouloumos commented at 7:43 pm on July 6, 2022:

Yes, I can’t understand what I was thinking going with that overcomplicated patch solution. Changing this to uint8_t allows me now to generate probes.h during build time.

I’ve still got issues with the build process on different OS resulting to failed checks, probably because of lack of experience, but I’ll figure it out.

in src/net_processing.cpp:4278 in 64d0862e31 outdated

4274@@ -4275,7 +4275,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
4275         pfrom->ConnectionTypeAsString().c_str(),
4276         msg.m_type.c_str(),
4277         msg.m_recv.size(),
4278-        msg.m_recv.data()
4279+        (unsigned char*)(msg.m_recv.data())

0xB10C commented at 7:43 pm on July 6, 2022:

Note to self: I want to check if this causes a difference in the number of instructions for this tracepoint. I think a reinterpret_cast could also work here as an alternative:

Unlike static_cast, but like const_cast, the reinterpret_cast expression does not compile to any CPU instructions [..]

DrahtBot commented at 3:33 am on July 7, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	michaelfolkson, brunoerg, 0xB10C, RandyMcMillan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

kouloumos force-pushed on Jul 7, 2022

tracing: macOS USDT support 6e6012a790

kouloumos force-pushed on Jul 7, 2022

0xB10C commented at 4:30 pm on July 7, 2022: contributor

I think, for testing the CI, a separate branch with CI (not the PR branch) is preferred. CirrusCI let’s you run it for free. Less noise on the PR and less notifications :)

kouloumos commented at 4:38 pm on July 7, 2022: contributor

I think, for testing the CI, a separate branch with CI (not the PR branch) is preferred. CirrusCI let’s you run it for free. Less noise on the PR and less notifications :)

Thanks for the suggestion! After the 6-7th push I’ve started thinking that this might be an issue😅 and I was wondering about solutions. Is this mentioned somewhere on the docs? ~You mean a separate branch on my own fork, right?~

Edit: It was so simple to setup, my apologies to all of you that got those 22 notifications.

build: fix detection of USDT support on macOS and generate `probes.h`

- Partially revert #22238 as after the macOS USDT support, just checking
for the `sys/sdt.h` header is enough.
- Generate `util/probes.h` during build time when supported.

4ff71a16bf

tracing: add dtrace example scripts

DTrace example scripts based on the existing bpftrace scripts.
Docs for macOS tracing support and example scripts.

2183f0308f

kouloumos marked this as ready for review on Jul 11, 2022

kouloumos force-pushed on Jul 11, 2022

kouloumos commented at 9:16 am on July 11, 2022: contributor

Ready for review!

util/probes.h is now generated during build time.
docs for the macOS tracing support and example scripts have been added.
The PR description has also been updated with more details.

RandyMcMillan commented at 3:10 am on September 10, 2022: contributor

Concept ACK

0xB10C commented at 12:59 pm on September 13, 2022: contributor

I want to revisit this soonish by trying to add support for the tracepoints from #25832 for macOS in a separate branch. It seems that this wouldn’t require too much changes.

kouloumos commented at 1:05 pm on September 13, 2022: contributor

I want to revisit this soonish by trying to add support for the tracepoints from #25832 for macOS in a separate branch. It seems that this wouldn’t require too much changes.

I’ve already did that with 6f3480e2044e4beae0d324c43ce6e0294f4915b3 on my macos-connection-tracepoints branch. I was planning to mention it but I didn’t manage to review #25832 yet.

0xB10C referenced this in commit 42a54e3436 on Nov 25, 2022

0xB10C referenced this in commit 2b21ff9a0f on Nov 25, 2022

0xB10C referenced this in commit 9f70c5dd82 on Nov 25, 2022

0xB10C referenced this in commit 91161d6859 on Nov 27, 2022

0xB10C referenced this in commit 27bd196214 on Nov 28, 2022

0xB10C referenced this in commit 94331ba3cc on Nov 28, 2022

0xB10C referenced this in commit 877e6a0581 on Nov 29, 2022

0xB10C referenced this in commit be0a187021 on Nov 29, 2022

0xB10C referenced this in commit 9bb85365d1 on Nov 29, 2022

0xB10C referenced this in commit 61ca0f99db on Nov 29, 2022

0xB10C referenced this in commit d20105e29c on Dec 20, 2022

0xB10C referenced this in commit bf997bcfe7 on Dec 20, 2022

0xB10C referenced this in commit 65366a5ae6 on Dec 20, 2022

0xB10C referenced this in commit c5ef919aa4 on Jan 4, 2023

0xB10C referenced this in commit 13714fda62 on Jan 4, 2023

0xB10C referenced this in commit 38df8dd065 on Mar 9, 2023

0xB10C referenced this in commit f92bb595d9 on Mar 9, 2023

kouloumos referenced this in commit 383eb5d7cf on Apr 8, 2023

kouloumos referenced this in commit d42aa93d7b on Apr 8, 2023

achow101 commented at 3:15 pm on April 25, 2023: member

What’s the status of this?

kouloumos commented at 12:30 pm on May 2, 2023: contributor

What’s the status of this?

It seems that there isn’t enough interest for this change. But anyway. with the current status of #26593, it doesn’t make sense for this PR to be dealt with before that one is merged. Additionally, there is a chance that #26593 will make it easier to reason for macOS USDT support if what I proposed gets into the final PR. Therefore, marking it as draft for now.

kouloumos marked this as a draft on May 2, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label CI failed on Jul 5, 2023

DrahtBot removed the label CI failed on Jul 7, 2023

DrahtBot added the label CI failed on Oct 25, 2023

DrahtBot removed the label CI failed on Oct 30, 2023

DrahtBot added the label CI failed on Nov 27, 2023

DrahtBot removed the label CI failed on Dec 10, 2023

DrahtBot added the label CI failed on Jan 1, 2024

DrahtBot removed the label CI failed on Jan 9, 2024

DrahtBot added the label CI failed on Jan 14, 2024

fanquake commented at 2:05 pm on March 6, 2024: member

Ok, going to close this for now, until these is sort of outcome out of #26593. Can be reopened if/when required.

fanquake closed this on Mar 6, 2024

tracing: macOS USDT support #25541

Overview

Notes

Adding tracepoints to Bitcoin Core (extra steps after this PR)

Reviews

Conflicts