Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 13, 2023

In the code we do not use string literals.

Also a check for DTRACE_PROBE7 macro has been added as not all systems defineDTRACE_PROBE{6,7,8,9,10,11,12} macros (e.g., FreeBSD).

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 13, 2023

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
ACK 0xB10C
Stale ACK vasild

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.

@hebasto
Copy link
Member Author

hebasto commented Apr 13, 2023

cc @0xB10C @vasild

@0xB10C
Copy link
Contributor

0xB10C commented Apr 13, 2023

Thanks for looking into this, however, I'm not sure if it's the right time for this. At the moment, we only really support the tracepoints on Linux.

The suggested commit fixes USDT detection [..]

The suggested commit enables USDT detection on FreeBSD, something that (somewhat unintentionally) wasn't enabled before. Not sure if it's worth enabling them at the moment for FreeBSD. It would be awesome to have them on more OS's (e.g. macOS see #25541), but AFAIK no one has looked into using them on FreeBSD.

@kouloumos suggested to use dtrace to generate the tracepoint macros in #26593 (review) which would unify the process for macOS and Linux. I just saw that there's a dtrace port for FreeBSD too. It would be nice if we could get FreeBSD, macOS and Linux with one unified approach. Until then, maybe leave them disabled?

@fanquake
Copy link
Member

I agree with @0xB10C here. Not sure about making this change, given the tracepoints still wont work anyways.

@hebasto
Copy link
Member Author

hebasto commented Apr 13, 2023

Besides the FreeBSD case, shouldn't tokens be passed to the tested macro rather than string literals?

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 468cf43

The actual source code does not pass strings and it is better to test for whatever the source code will use. Maybe adjust the PR title/OP and the commit message.

AFAIK no one has looked into using them on FreeBSD

I would use them, please :)

I just saw that there's a dtrace port for FreeBSD too

DTrace has been part of the FreeBSD base system since 2009. This is why I found it strange that Bitcoin Core's src/util/trace.h uses e.g. DTRACE_PROBE3(), but that is not supported on FreeBSD. The man page: STD(9).

@fanquake
Copy link
Member

What are we doing here?

If we aren't doing this yet, I think it can just be closed and/or combined in #26593. Otherwise,

Maybe adjust the PR title/OP and the commit message.

?

@hebasto hebasto changed the title build: Fix USDT detection on FreeBSD build: Detect USDT the same way how it is used in the code May 18, 2023
@hebasto hebasto marked this pull request as ready for review May 18, 2023 14:04
@hebasto
Copy link
Member Author

hebasto commented May 18, 2023

What are we doing here?

Reworked. The PR description has been updated.

@0xB10C
Copy link
Contributor

0xB10C commented May 18, 2023

ACK b53cab0

Tracepoints for FreeBSD can happen at a later point, if needed.

@DrahtBot DrahtBot requested a review from vasild May 18, 2023 17:35
@fanquake fanquake merged commit 2f1403a into bitcoin:master May 19, 2023
@hebasto hebasto deleted the 230413-freebsd branch May 19, 2023 09:46
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 19, 2023
…n the code

b53cab0 build: Detect USDT the same way how it is used in the code (Hennadii Stepanov)

Pull request description:

  In the code we do not use string literals.

  Also a check for `DTRACE_PROBE7` macro has been added as not all systems define`DTRACE_PROBE{6,7,8,9,10,11,12}` macros (e.g., FreeBSD).

ACKs for top commit:
  0xB10C:
    ACK b53cab0

Tree-SHA512: 74f49424d57bf1929f2b09edba1449cef5a1a2448161952da35302343f3003d5bedeab1417e166b656c5f629303e2de888550b1219e886a1b991b12b9c880794
@bitcoin bitcoin locked and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants