build: Detect USDT the same way how it is used in the code #27458

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:230413-freebsd changing 1 files +3 −1
  1. hebasto commented at 8:47 am on April 13, 2023: member

    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).

  2. DrahtBot commented at 8:47 am on April 13, 2023: 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
    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.

  3. DrahtBot added the label Build system on Apr 13, 2023
  4. hebasto commented at 8:47 am on April 13, 2023: member
    cc @0xB10C @vasild
  5. 0xB10C commented at 9:16 am on April 13, 2023: contributor

    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#pullrequestreview-1376804705 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?

  6. fanquake commented at 9:22 am on April 13, 2023: member
    I agree with @0xB10C here. Not sure about making this change, given the tracepoints still wont work anyways.
  7. hebasto commented at 9:45 am on April 13, 2023: member
    Besides the FreeBSD case, shouldn’t tokens be passed to the tested macro rather than string literals?
  8. vasild approved
  9. vasild commented at 11:15 am on April 17, 2023: contributor

    ACK 468cf43ce8c38d6d92506fe22446d04e147e6786

    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).

  10. fanquake commented at 11:06 am on May 18, 2023: 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.

    ?

  11. build: Detect USDT the same way how it is used in the code b53cab0083
  12. hebasto force-pushed on May 18, 2023
  13. hebasto renamed this:
    build: Fix USDT detection on FreeBSD
    build: Detect USDT the same way how it is used in the code
    on May 18, 2023
  14. hebasto marked this as ready for review on May 18, 2023
  15. hebasto commented at 2:04 pm on May 18, 2023: member

    What are we doing here?

    Reworked. The PR description has been updated.

  16. 0xB10C commented at 5:35 pm on May 18, 2023: contributor

    ACK b53cab0083d99e9610d74517d1d41fc615953770

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

  17. DrahtBot requested review from vasild on May 18, 2023
  18. fanquake merged this on May 19, 2023
  19. fanquake closed this on May 19, 2023
  20. hebasto deleted the branch on May 19, 2023
  21. sidhujag referenced this in commit 698b5af203 on May 19, 2023
  22. bitcoin locked this on May 18, 2024


hebasto DrahtBot 0xB10C fanquake vasild


vasild

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me