Skip to content
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

[PROF-10967] Fix profiler not loading due to "rb_obj_info" symbol not found #4161

Merged
merged 3 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test-memory-leaks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ jobs:
- uses: actions/checkout@v4
- uses: ruby/setup-ruby@v1
with:
ruby-version: 3.4.0-preview1 # TODO: Use stable version once 3.4 is out
ruby-version: 3.4.0-preview2 # TODO: Use stable version once 3.4 is out
bundler-cache: true # runs 'bundle install' and caches installed gems automatically
bundler: latest
cache-version: v1 # bump this to invalidate cache
Expand Down
3 changes: 3 additions & 0 deletions ext/datadog_profiling_native_extension/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ def skip_building_extension!(reason)

have_func "malloc_stats"

# On Ruby 2.5 and 3.3, this symbol was not visible. It is on 2.6 to 3.2, as well as 3.4+
$defs << "-DNO_RB_OBJ_INFO" if RUBY_VERSION.start_with?("2.5", "3.3")

# On older Rubies, rb_postponed_job_preregister/rb_postponed_job_trigger did not exist
$defs << "-DNO_POSTPONED_TRIGGER" if RUBY_VERSION < "3.3"

Expand Down
10 changes: 2 additions & 8 deletions ext/datadog_profiling_native_extension/private_vm_api_access.c
Original file line number Diff line number Diff line change
Expand Up @@ -587,16 +587,13 @@ int ddtrace_rb_profile_frames(VALUE thread, int start, int limit, frame_info *st
// Taken from upstream vm_insnhelper.c at commit 5f10bd634fb6ae8f74a4ea730176233b0ca96954 (March 2022, Ruby 3.2 trunk)
// Copyright (C) 2007 Koichi Sasada
// to support our custom rb_profile_frames (see above)
// Modifications: None
// Modifications:
// * Removed debug checks (they were ifdef'd out anyway)
static rb_callable_method_entry_t *
check_method_entry(VALUE obj, int can_be_svar)
{
if (obj == Qfalse) return NULL;

#if VM_CHECK_MODE > 0
if (!RB_TYPE_P(obj, T_IMEMO)) rb_bug("check_method_entry: unknown type: %s", rb_obj_info(obj));
#endif

switch (imemo_type(obj)) {
case imemo_ment:
return (rb_callable_method_entry_t *)obj;
Expand All @@ -608,9 +605,6 @@ check_method_entry(VALUE obj, int can_be_svar)
}
// fallthrough
default:
#if VM_CHECK_MODE > 0
rb_bug("check_method_entry: svar should not be there:");
#endif
return NULL;
}
}
Expand Down
6 changes: 6 additions & 0 deletions ext/datadog_profiling_native_extension/profiling.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ static VALUE _native_trigger_holding_the_gvl_signal_handler_on(DDTRACE_UNUSED VA
static VALUE _native_enforce_success(DDTRACE_UNUSED VALUE _self, VALUE syserr_errno, VALUE with_gvl);
static void *trigger_enforce_success(void *trigger_args);
static VALUE _native_malloc_stats(DDTRACE_UNUSED VALUE _self);
static VALUE _native_safe_object_info(DDTRACE_UNUSED VALUE _self, VALUE obj);

void DDTRACE_EXPORT Init_datadog_profiling_native_extension(void) {
VALUE datadog_module = rb_define_module("Datadog");
Expand Down Expand Up @@ -72,6 +73,7 @@ void DDTRACE_EXPORT Init_datadog_profiling_native_extension(void) {
rb_define_singleton_method(testing_module, "_native_trigger_holding_the_gvl_signal_handler_on", _native_trigger_holding_the_gvl_signal_handler_on, 1);
rb_define_singleton_method(testing_module, "_native_enforce_success", _native_enforce_success, 2);
rb_define_singleton_method(testing_module, "_native_malloc_stats", _native_malloc_stats, 0);
rb_define_singleton_method(testing_module, "_native_safe_object_info", _native_safe_object_info, 1);
}

static VALUE native_working_p(DDTRACE_UNUSED VALUE _self) {
Expand Down Expand Up @@ -265,3 +267,7 @@ static VALUE _native_malloc_stats(DDTRACE_UNUSED VALUE _self) {
return Qfalse;
#endif
}

static VALUE _native_safe_object_info(DDTRACE_UNUSED VALUE _self, VALUE obj) {
return rb_str_new_cstr(safe_object_info(obj));
}
18 changes: 14 additions & 4 deletions ext/datadog_profiling_native_extension/ruby_helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "ruby_helpers.h"
#include "private_vm_api_access.h"
#include "extconf.h"

// The following global variables are initialized at startup to save expensive lookups later.
// They are not expected to be mutated outside of init.
Expand Down Expand Up @@ -219,17 +220,26 @@ static bool ruby_is_obj_with_class(VALUE obj) {
return false;
}

// These two functions are not present in the VM headers, but are public symbols that can be invoked.
// This function is not present in the VM headers, but is a public symbol that can be invoked.
int rb_objspace_internal_object_p(VALUE obj);
const char *rb_obj_info(VALUE obj);

#ifdef NO_RB_OBJ_INFO
const char* safe_object_info(DDTRACE_UNUSED VALUE obj) { return "(No rb_obj_info for current Ruby)"; }
#else
// This function is a public symbol, but not on all Rubies; `safe_object_info` below abstracts this, and
// should be used instead.
const char *rb_obj_info(VALUE obj);

const char* safe_object_info(VALUE obj) { return rb_obj_info(obj); }
#endif

VALUE ruby_safe_inspect(VALUE obj) {
if (!ruby_is_obj_with_class(obj)) return rb_str_new_cstr("(Not an object)");
if (rb_objspace_internal_object_p(obj)) return rb_sprintf("(VM Internal, %s)", rb_obj_info(obj));
if (rb_objspace_internal_object_p(obj)) return rb_sprintf("(VM Internal, %s)", safe_object_info(obj));
// @ivoanjo: I saw crashes on Ruby 3.1.4 when trying to #inspect matchdata objects. I'm not entirely sure why this
// is needed, but since we only use this method for debug purposes I put in this alternative and decided not to
// dig deeper.
if (rb_type(obj) == RUBY_T_MATCH) return rb_sprintf("(MatchData, %s)", rb_obj_info(obj));
if (rb_type(obj) == RUBY_T_MATCH) return rb_sprintf("(MatchData, %s)", safe_object_info(obj));
if (rb_respond_to(obj, inspect_id)) return rb_sprintf("%+"PRIsVALUE, obj);
if (rb_respond_to(obj, to_s_id)) return rb_sprintf("%"PRIsVALUE, obj);

Expand Down
4 changes: 4 additions & 0 deletions ext/datadog_profiling_native_extension/ruby_helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,7 @@ size_t ruby_obj_memsize_of(VALUE obj);
// return a string with the result of that call. Elsif the object responds to
// 'to_s', return a string with the result of that call. Otherwise, return Qnil.
VALUE ruby_safe_inspect(VALUE obj);

// You probably want ruby_safe_inspect instead; this is a lower-level dependency
// of it, that's being exposed here just to facilitate testing.
const char* safe_object_info(VALUE obj);
22 changes: 22 additions & 0 deletions spec/datadog/profiling/native_extension_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,4 +183,26 @@
end
end
end

describe "safe_object_info" do
let(:object_to_inspect) { "Hey, I'm a string!" }

subject(:safe_object_info) { described_class::Testing._native_safe_object_info(object_to_inspect) }

context "on a Ruby with rb_obj_info" do
before { skip "Behavior does not apply to current Ruby version" if RUBY_VERSION.start_with?("2.5", "3.3") }

it "returns a string with information about the object" do
expect(safe_object_info).to include("T_STRING")
end
end

context "on a Ruby without rb_obj_info" do
before { skip "Behavior does not apply to current Ruby version" unless RUBY_VERSION.start_with?("2.5", "3.3") }

it "returns a placeholder string and does not otherwise fail" do
expect(safe_object_info).to eq "(No rb_obj_info for current Ruby)"
end
end
end
end
Loading