Skip to content

Commit

Permalink
Merge pull request #4161 from DataDog/ivoanjo/prof-10967-fix-rb-obj-i…
Browse files Browse the repository at this point in the history
…nfo-usage

[PROF-10967] Fix profiler not loading due to "rb_obj_info" symbol not found
  • Loading branch information
ivoanjo authored and TonyCTHsu committed Nov 28, 2024
1 parent 72d42da commit 550b57e
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 13 deletions.
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

0 comments on commit 550b57e

Please sign in to comment.