Merge pull request #4161 from DataDog/ivoanjo/prof-10967-fix-rb-obj-info-usage

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

Author: ivoanjo

.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

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"
 

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;
     }
 }

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));
+}

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);
 

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);

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