Skip to content

[NO-TICKET] Minor: Fix checking for dladdr in profiling #4783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 9, 2025

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 7, 2025

What does this PR do?

This PR fixes the checks for the dladdr/dladdr1 function added in #4745 to actually check the values of the defines.

Specifically, the have_... helpers in extconf.rb behave differently from the other defines we set up.

That is, have_func("dladdr") will always define HAVE_DLADDR, even though it might define it to 0 (not available). (Our other checks only define things when they should be used, e.g.
$defs << "-DNO_POSTPONED_TRIGGER" if RUBY_VERSION < "3.3").

This means that it's not correct to use #ifdef HAVE_DLADDR to check if HAVE_DLADDR exists and is == 1, we actually need to test it separately.

In this very specific case, in practice, I suspect we won't trigger issues because our extconf.rb is

if have_header("dlfcn.h")
  (have_struct_member("struct link_map", "l_name", "link.h") && have_func("dladdr1")) ||
    have_func("dladdr")
end

...and thus if the header doesn't exist, we won't run the other checks, so I think only in a situation where the header exists and doesn't have the function would there be a problem (and I'm not sure that can happen on Linux).

Anyway, I believe it's still worth fixing as this is a footgun waiting to happen, and compilation errors are always mega annoying.

Motivation:

Fix potential compilation error when installing profiling.

Change log entry

Yes. Fix potential profiler compilation issue.

Additional Notes:

I've actually ran into this in the past in
ruby/ruby@7472fff

How to test the change?

If CI is green, this should be good!

**What does this PR do?**

This PR fixes the checks for the `dladdr`/`dladdr1` function added in
#4745 to actually check the
values of the defines.

Specifically, the `have_...` helpers in `extconf.rb` behave
differently from the other defines we set up.

That is, `have_func("dladdr")` will always define `HAVE_DLADDR`, even
though it might define it to `0` (not available). (Our other checks
only define things when they should be used, e.g.
`$defs << "-DNO_POSTPONED_TRIGGER" if RUBY_VERSION < "3.3"`).

This means that it's not correct to use `#ifdef HAVE_DLADDR` to check
if `HAVE_DLADDR` exists and is == 1, we actually need to test it
separately.

In this very specific case, in practice, I suspect we won't trigger
issues because our extconf.rb is

```ruby
if have_header("dlfcn.h")
  (have_struct_member("struct link_map", "l_name", "link.h") && have_func("dladdr1")) ||
    have_func("dladdr")
end
```

...and thus if the header doesn't exist, we won't run the other checks,
so I think only in a situation where the header exists and doesn't have
the function would there be a problem (and I'm not sure that can happen
on Linux).

Anyway, I believe it's still worth fixing as this is a footgun waiting
to happen, and compilation errors are always mega annoying.

**Motivation:**

Fix potential compilation error when installing profiling.

**Additional Notes:**

I've actually ran into this in the past in
ruby/ruby@7472fff

**How to test the change?**

If CI is green, this should be good!
@ivoanjo ivoanjo requested review from a team as code owners July 7, 2025 08:34
@github-actions github-actions bot added the profiling Involves Datadog profiling label Jul 7, 2025
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.52%. Comparing base (d903ac5) to head (05a6cea).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4783      +/-   ##
==========================================
- Coverage   97.54%   97.52%   -0.02%     
==========================================
  Files        1483     1483              
  Lines       88562    88561       -1     
  Branches     4591     4591              
==========================================
- Hits        86384    86370      -14     
- Misses       2178     2191      +13     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter
Copy link

pr-commenter bot commented Jul 7, 2025

Benchmarks

Benchmark execution time: 2025-07-07 10:59:28

Comparing candidate commit 05a6cea in PR branch ivoanjo/minor-fix-checking-for-dladdr with baseline commit d903ac5 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 6 unstable metrics.

Copy link
Member

@p-datadog p-datadog left a comment

Choose a reason for hiding this comment

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

Would it be less error-prone to undef these defines if they are set to zero?

@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 7, 2025

Would it be less error-prone to undef these defines if they are set to zero?

I think undef would be a bit confusing TBH... If we went down that route, I think it would be preferable to perhaps ignore Ruby's defines and use our own, e.g. do something like

if have_func("dladdr1")
  $defs << "-DUSE_DLADDR1" 
elsif have_func("dladdr")
  $defs << "-DUSE_DLADDR" 
end

But it's not amazingly clean either. Any specific preference?

@ivoanjo
Copy link
Member Author

ivoanjo commented Jul 9, 2025

I'm going to go ahead and merge this one -- happy to change the strategy on a follow-up once we decide what option to use.

@ivoanjo ivoanjo merged commit 54d2e48 into master Jul 9, 2025
446 of 447 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/minor-fix-checking-for-dladdr branch July 9, 2025 08:09
@github-actions github-actions bot added this to the 2.19.0 milestone Jul 9, 2025
@lloeki lloeki mentioned this pull request Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants