From a257c35974b01a99ada78fbcfded8d2d8ba195e8 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Tue, 26 Nov 2024 12:32:05 +0000 Subject: [PATCH 1/3] [PROF-10967] Fix profiler not loading due to "rb_obj_info" symbol not found **What does this PR do?** This PR fixes a bug introduced in #4020, specifically in f581076c0f927 . We started using the `rb_obj_info` to print debug information about objects in some cases, BUT I failed to notice that this API is not really available on Ruby 2.5 and 3.3 (but is on all others, which is why it tripped me). This manifested in the following error reported by a customer: > WARN -- datadog: [datadog] Profiling was requested but is not > supported, profiling disabled: There was an error loading the > profiling native extension due to 'RuntimeError Failure to load > datadog_profiling_native_extension.3.3.5_x86_64-linux-musl due > to Error relocating > /app/vendor/bundle/ruby/3.3.0/gems/datadog-2.6.0/lib/datadog/profiling/../../datadog_profiling_native_extension.3.3.5_x86_64-linux-musl.so: > rb_obj_info: symbol not found' at > '/app/vendor/bundle/ruby/3.3.0/gems/datadog-2.6.0/lib/datadog/profiling/load_native_extension.rb:41:in `
'' This PR fixes this issue by never referencing `rb_obj_info` on those Rubies. Since this API is only used for printing information during errors, this should be fine (and is better than the alternative of not printing info on any Rubies). **Motivation:** Fix profiling not loading in certain situations on Ruby 2.5 and 3.3. **Additional Notes:** Interestingly, this issue did not show up on glibc systems. I guess musl libc is being a bit more eager about trying to resolve symbols? **How to test the change?** This change includes test coverage. Disabling the added check in `extconf.rb` will produce a failing test. --- .../extconf.rb | 3 +++ .../private_vm_api_access.c | 10 ++------- .../profiling.c | 6 +++++ .../ruby_helpers.c | 18 +++++++++++---- .../ruby_helpers.h | 4 ++++ .../profiling/native_extension_spec.rb | 22 +++++++++++++++++++ 6 files changed, 51 insertions(+), 12 deletions(-) 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..52ac9cb16e9 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 eq "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 From 6518e65a191561efcbb5a4b9c7d2ecdd8fcb60c7 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Tue, 26 Nov 2024 12:54:20 +0000 Subject: [PATCH 2/3] Tweak test to account for Ruby 3.1 including more info in rb_obj_info --- spec/datadog/profiling/native_extension_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/datadog/profiling/native_extension_spec.rb b/spec/datadog/profiling/native_extension_spec.rb index 52ac9cb16e9..b3fa0c8163a 100644 --- a/spec/datadog/profiling/native_extension_spec.rb +++ b/spec/datadog/profiling/native_extension_spec.rb @@ -193,7 +193,7 @@ 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 eq "T_STRING" + expect(safe_object_info).to include("T_STRING") end end From 94104b8e65abb97500d65a88a04d562d4d5ad478 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Tue, 26 Nov 2024 12:55:23 +0000 Subject: [PATCH 3/3] Bump test-memcheck job to use 3.4.0-preview2 The `rb_obj_info` got re-exposed later in the 3.4 development cycle, so our test that was still running with 3.4.0-preview1 was failing because it still had not been changed. --- .github/workflows/test-memory-leaks.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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