-
Notifications
You must be signed in to change notification settings - Fork 399
[PROF-12743] Runtime stack collection callback registration #4984
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Hey @DataDog/ruby-guild, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md(possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None.(possible answers No/Nope/None) Visited at: 2025-11-18 21:06:17 UTC |
Typing analysisIgnored filesThis PR introduces 1 ignored file. It decreases the percentage of typed files from 38.32% to 38.28% (-0.04%). Ignored files (+1-0)❌ Introduced:Note: Ignored files are excluded from the next sections. Untyped other declarationsThis PR clears 1 untyped other declaration. It decreases the percentage of typed other declarations from 68.02% to 67.96% (-0.06%). Untyped other declarations (+0-1)✅ Cleared: |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 6e6e165 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
e628919 to
564c828
Compare
f102a46 to
53afc20
Compare
|
Paper trail [ ] Checking unreasonable string sizes |
77d1391 to
1ebb325
Compare
1ebb325 to
4e51bf3
Compare
3c782d0 to
8d957bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d056dfc to
95fcf25
Compare
7b89539 to
3c6b8ae
Compare
a254d60 to
1401c5f
Compare
47cf97a to
92758fd
Compare
92758fd to
6c25fde
Compare
ivoanjo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it a big pass! I think in general this seems reasonable -- most of my thoughts/worries are around ruby_runtime_stack_callback and making sure we get it into shape.
| // Check if the heap object pointed to by str is readable | ||
| if (!is_pointer_readable((const void *)str, sizeof(struct RBasic))) return false; | ||
|
|
||
| // For strings, we need to check the full RString structure | ||
| if (!is_pointer_readable(RSTRING(str), sizeof(struct RString))) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this AI...? This seems a redundant check...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be due to my unfamiliarity with Ruby internals, but the first check is intended to ensure the VALUE actually points to a live struct RBasic. Without the first check, even touching RSTRING(str) would dereference garbage and crash.The second check validates the string-specific payload (struct RString) since Ruby string’s header extends RBasic with extra fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... In fact RSTRING(...) is just a cast so that's why I was confused:
/**
* Convenient casting macro.
*
* @param obj An object, which is in fact an ::RString.
* @return The passed object casted to ::RString.
*/
#define RSTRING(obj) RBIMPL_CAST((struct RString *)(obj))but TBH it's probably best to avoid as much as possible Ruby macros and other calls here to avoid relying on too many assumptions of what they do.
That is -- this should probably be a is_pointer_readable((const void *)str, sizeof(struct RString)) and thus avoid the reliance on Ruby headers other than to know "what's the size of it".
| static bool is_valid_control_frame(const rb_control_frame_t *cfp, | ||
| const rb_execution_context_t *ec) { | ||
| if (!cfp) return false; | ||
|
|
||
| void *stack_start = ec->vm_stack; | ||
| void *stack_end = (char*)stack_start + ec->vm_stack_size * sizeof(VALUE); | ||
| if ((void*)cfp < stack_start || (void*)cfp >= stack_end) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!is_pointer_readable(cfp, sizeof(rb_control_frame_t))) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're validating some pointers, but not others... Why?
| static void ruby_runtime_stack_callback( | ||
| void (*emit_frame)(const ddog_crasht_RuntimeStackFrame*) | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this function is very complex, because it also does a very complex thing. My question is -- what happens if we get something wrong here? That is, we already have a bunch of safeguards, but what can actually happen if we miss a spot in pratice? Is there a safety net for us, or not?
| function_name = safe_string_ptr(name); | ||
| } | ||
|
|
||
| VALUE filename = rb_iseq_path(iseq); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this slightly makes me nervous, and comes back to my question of "do we have a safety net for this or not". We're asking the VM to get something for us and thus we have no control over valid pointers and the VM doing sane things if something is wrong...
c7a819c to
ef62258
Compare

What does this PR do?
We want to collect runtime frames for the crashtracker. This approach follows how we access the call stack for profiling.
Motivation:
Change log entry
Additional Notes:
How to test the change?
"Ruby and C method runtime stack capture"RSpec testRun a ruby program with the crashtracker initialized. Runtime stacks should be visible in the
experimentalsection of the crash report. Stacktrace emitted from an example script I wrote to test