diff --git a/.github/workflows/test-memory-leaks.yaml b/.github/workflows/test-memory-leaks.yaml index 90100cb2147..e485c4a545f 100644 --- a/.github/workflows/test-memory-leaks.yaml +++ b/.github/workflows/test-memory-leaks.yaml @@ -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 diff --git a/ext/datadog_profiling_native_extension/extconf.rb b/ext/datadog_profiling_native_extension/extconf.rb index fb3e9edc573..4acadd68215 100644 --- a/ext/datadog_profiling_native_extension/extconf.rb +++ b/ext/datadog_profiling_native_extension/extconf.rb @@ -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" diff --git a/ext/datadog_profiling_native_extension/private_vm_api_access.c b/ext/datadog_profiling_native_extension/private_vm_api_access.c index af78a027d30..dbbebd531d0 100644 --- a/ext/datadog_profiling_native_extension/private_vm_api_access.c +++ b/ext/datadog_profiling_native_extension/private_vm_api_access.c @@ -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; @@ -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; } } diff --git a/ext/datadog_profiling_native_extension/profiling.c b/ext/datadog_profiling_native_extension/profiling.c index 96637cd110c..315ca0d2954 100644 --- a/ext/datadog_profiling_native_extension/profiling.c +++ b/ext/datadog_profiling_native_extension/profiling.c @@ -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"); @@ -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) { @@ -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)); +} diff --git a/ext/datadog_profiling_native_extension/ruby_helpers.c b/ext/datadog_profiling_native_extension/ruby_helpers.c index 084dadf9e67..09b14d20855 100644 --- a/ext/datadog_profiling_native_extension/ruby_helpers.c +++ b/ext/datadog_profiling_native_extension/ruby_helpers.c @@ -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. @@ -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); diff --git a/ext/datadog_profiling_native_extension/ruby_helpers.h b/ext/datadog_profiling_native_extension/ruby_helpers.h index ee609f11b86..fd38c400b7a 100644 --- a/ext/datadog_profiling_native_extension/ruby_helpers.h +++ b/ext/datadog_profiling_native_extension/ruby_helpers.h @@ -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); diff --git a/spec/datadog/profiling/native_extension_spec.rb b/spec/datadog/profiling/native_extension_spec.rb index f68ace2a796..b3fa0c8163a 100644 --- a/spec/datadog/profiling/native_extension_spec.rb +++ b/spec/datadog/profiling/native_extension_spec.rb @@ -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