Merge pull request #1740 from DataDog/ivoanjo/prof-4198-native-cpu-ti…

…me-on-older-rubies

[PROF-4198] Gather CPU time in profiler without monkey patching Thread for all supported Ruby versions (2.1 to 3.x)

.github/workflows/test-cross-platform.yaml

@@ -26,7 +26,7 @@ jobs:
       fail-fast: false
       matrix:
         os: [windows-latest]
-        ruby: [2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, '3.0', head]
+        ruby: [2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, '3.0']
     runs-on: ${{ matrix.os }}
     env:
       SKIP_SIMPLECOV: 1

.rubocop.yml

@@ -375,3 +375,6 @@ Style/TrailingCommaInHashLiteral:
 Style/GlobalVars:
   Exclude:
     - 'ext/ddtrace_profiling_native_extension/extconf.rb'
+
+Naming/VariableNumber:
+  Enabled: false

ddtrace.gemspec

@@ -46,8 +46,11 @@ Gem::Specification.new do |spec|
     spec.add_dependency 'msgpack', '< 1.4'
   end
 
-  # Used by the profiler
-  spec.add_dependency 'ffi', '~> 1.0'
+  # Used by the profiler native extension to support older Rubies (see NativeExtensionDesign.md for notes)
+  #
+  # Because we only use this for older Rubies, and we consider it "feature-complete" for those older Rubies,
+  # we're pinning it at the latest available version and will manually bump the dependency as needed.
+  spec.add_dependency 'debase-ruby_core_source', '= 0.10.12'
 
   spec.extensions = ['ext/ddtrace_profiling_native_extension/extconf.rb']
 end

ext/ddtrace_profiling_native_extension/NativeExtensionDesign.md

@@ -21,8 +21,8 @@ in future releases -- e.g. disabling the extension will disable profiling entire
 The profiling native extension is (and must always be) designed to **not cause failures** during gem installation, even
 if some features, Ruby versions, or operating systems are not supported.
 
-E.g. the extension must cleanly build on Ruby 2.1 on Windows, even if at run time it will effectively do nothing for
-such a setup.
+E.g. the extension must cleanly build on Ruby 2.1 (or the oldest Ruby version we support at the time) on Windows,
+even if at run time it will effectively do nothing for such a setup.
 
 We have a CI setup to help validate this, but this is really important to keep in mind when adding to or changing the
 existing codebase.
@@ -32,17 +32,35 @@ existing codebase.
 To implement some of the features below, we sometimes require access to private Ruby header files (that describe VM
 internal types, structures and functions).
 
-Because these private header files are not included in regular Ruby installations, our current workaround is to piggy
-back on a special header that Ruby includes that is only intended for use by the Ruby MJIT compiler. This header is
-placed inside the `include/` directory in a Ruby installation, and is named for that specific Ruby version. e.g.
-`rb_mjit_min_header-2.7.4.h`.
+Because these private header files are not included in regular Ruby installations, we have two different workarounds:
 
-This header was introduced by the first Ruby version that added MJIT, which is 2.6+.
-
-For older Ruby versions (see safety section above), this header is not available, and this must be handled gracefully.
+1. for Ruby versions >= 2.6 we make use use the Ruby private MJIT header
+2. for Ruby versions < 2.6 (legacy Rubies) we make use of the `debase-ruby_core_source` gem
 
 Functions which make use of these headers are defined in the <private_vm_api_acccess.c> file.
 
+**Important Note**: Our medium/long-term plan is to stop relying on all private Ruby headers, and instead request and
+contribute upstream changes so that they become official public VM APIs.
+
+### Approach 1: Using the Ruby private MJIT header
+
+Ruby versions >= 2.6 introduced a JIT compiler called MJIT. This compiler does not directly generate machine code;
+instead it generates C code and uses the system C compiler to turn it into machine code.
+
+The generated C code `#include`s a private header -- which we reference as "the MJIT header" everywhere.
+The MJIT header gets shipped with all MJIT-enabled Rubies and includes the layout of many internal VM structures;
+and of course the intention is that it is only used by the Ruby MJIT compiler.
+
+This header is placed inside the `include/` directory in a Ruby installation, and is named for that specific Ruby
+version. e.g. `rb_mjit_min_header-2.7.4.h`.
+
+### Approach 2: Using the `debase-ruby_core_source` gem
+
+The [`debase-ruby_core_source`](https://github.com/ruby-debug/debase-ruby_core_source) contains almost no code;
+instead, it just contains per-Ruby-version folders with the private VM headers (`.h`) files for that version.
+
+Thus, even though a regular Ruby installation does not include these files, we can access the copy inside this gem.
+
 ## Feature: Getting thread CPU-time clock_ids
 
 * **OS support**: Linux

ext/ddtrace_profiling_native_extension/clock_id_from_pthread.c

@@ -2,14 +2,19 @@
 
 // This file is only compiled on systems where pthread_getcpuclockid() is available;
 // Otherwise we compile clock_id_noop.c
-#if defined(HAVE_PTHREAD_GETCPUCLOCKID) && defined(USE_MJIT_HEADER)
+#ifdef HAVE_PTHREAD_GETCPUCLOCKID
 
 #include <pthread.h>
 #include <time.h>
 #include <errno.h>
 
 #include <ruby.h>
+
+#ifdef RUBY_2_1_WORKAROUND
+#include <thread_native.h>
+#else
 #include <ruby/thread_native.h>
+#endif
 
 #include "private_vm_api_access.h"
 

ext/ddtrace_profiling_native_extension/clock_id_noop.c

@@ -2,7 +2,7 @@
 
 // This file is the dual of clock_id_from_pthread.c for systems where that info
 // is not available.
-#if !(defined(HAVE_PTHREAD_GETCPUCLOCKID) && defined(USE_MJIT_HEADER))
+#ifndef HAVE_PTHREAD_GETCPUCLOCKID
 
 #include <ruby.h>
 

ext/ddtrace_profiling_native_extension/extconf.rb

@@ -1,16 +1,20 @@
-# typed: false
+# typed: ignore
 
 def skip_building_extension?
   # We don't support JRuby for profiling, and JRuby doesn't support native extensions, so let's just skip this entire
   # thing so that JRuby users of dd-trace-rb aren't impacted.
   on_jruby = RUBY_ENGINE == 'jruby'
 
+  # We don't officially support TruffleRuby for dd-trace-rb at all BUT let's not break adventurous customers that
+  # want to give it a try.
+  on_truffleruby = RUBY_ENGINE == 'truffleruby'
+
   # Experimental toggle to disable building the extension.
   # Disabling the extension will lead to the profiler not working in future releases.
   # If you needed to use this, please tell us why on <https://github.com/DataDog/dd-trace-rb/issues/new>.
   disabled_via_env = ENV['DD_PROFILING_NO_EXTENSION'].to_s.downcase == 'true'
 
-  on_jruby || disabled_via_env
+  on_jruby || on_truffleruby || disabled_via_env
 end
 
 # IMPORTANT: When adding flags, remember that our customers compile with a wide range of gcc/clang versions, so
@@ -20,11 +24,19 @@ def add_compiler_flag(flag)
 end
 
 if skip_building_extension?
+  # rubocop:disable Style/StderrPuts
+  $stderr.puts(%(
++------------------------------------------------------------------------------+
+| Skipping build of profiling native extension and replacing it with a no-op   |
+| Makefile                                                                     |
++------------------------------------------------------------------------------+
+
+))
+
   File.write('Makefile', 'all install clean: # dummy makefile that does nothing')
   return
 end
 
-# rubocop:disable Style/StderrPuts
 $stderr.puts(%(
 +------------------------------------------------------------------------------+
 | **Preparing to build the ddtrace native extension...**                       |
@@ -40,6 +52,7 @@ def add_compiler_flag(flag)
 |                                                                              |
 | Thanks for using ddtrace! You rock!                                          |
 +------------------------------------------------------------------------------+
+
 ))
 # rubocop:enable Style/StderrPuts
 
@@ -57,10 +70,6 @@ def add_compiler_flag(flag)
 # cause a segfault later. Let's ensure that never happens.
 add_compiler_flag '-Werror-implicit-function-declaration'
 
-# Older Rubies don't have the MJIT header (used by the JIT compiler, and we piggy back on it)
-# TODO: Development builds of Ruby 3.1 seem to be failing on Windows; to be revisited once 3.1.0 stable is out
-$defs << '-DUSE_MJIT_HEADER' unless RUBY_VERSION < '2.6' || (RUBY_VERSION >= '3.1' && Gem.win_platform?)
-
 if RUBY_PLATFORM.include?('linux')
   # Supposedly, the correct way to do this is
   # ```
@@ -72,24 +81,53 @@ def add_compiler_flag(flag)
   $defs << '-DHAVE_PTHREAD_GETCPUCLOCKID'
 end
 
-create_header
-
-# The MJIT header is always (afaik?) suffixed with the exact Ruby VM version,
-# including patch (e.g. 2.7.2). Thus, we add to the header file a definition
-# containing the exact file, so that it can be used in a #include in the C code.
-header_contents =
-  File.read($extconf_h)
-      .sub('#endif',
-           <<-EXTCONF_H.strip
-#define RUBY_MJIT_HEADER "rb_mjit_min_header-#{RUBY_VERSION}.h"
-
-#endif
-           EXTCONF_H
-          )
-File.open($extconf_h, 'w') { |file| file.puts(header_contents) }
+# Older Rubies don't have the MJIT header, used by the JIT compiler, so we need to use a different approach
+CAN_USE_MJIT_HEADER = RUBY_VERSION >= '2.6'
 
 # Tag the native extension library with the Ruby version and Ruby platform.
 # This makes it easier for development (avoids "oops I forgot to rebuild when I switched my Ruby") and ensures that
 # the wrong library is never loaded.
 # When requiring, we need to use the exact same string, including the version and the platform.
-create_makefile "ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}"
+EXTENSION_NAME = "ddtrace_profiling_native_extension.#{RUBY_VERSION}_#{RUBY_PLATFORM}".freeze
+
+if CAN_USE_MJIT_HEADER
+  $defs << '-DUSE_MJIT_HEADER'
+
+  # NOTE: This needs to come after all changes to $defs
+  create_header
+
+  # The MJIT header is always (afaik?) suffixed with the exact Ruby VM version,
+  # including patch (e.g. 2.7.2). Thus, we add to the header file a definition
+  # containing the exact file, so that it can be used in a #include in the C code.
+  header_contents =
+    File.read($extconf_h)
+        .sub('#endif',
+             <<-EXTCONF_H.strip
+#define RUBY_MJIT_HEADER "rb_mjit_min_header-#{RUBY_VERSION}.h"
+
+#endif
+             EXTCONF_H
+            )
+  File.open($extconf_h, 'w') { |file| file.puts(header_contents) }
+
+  create_makefile EXTENSION_NAME
+else
+  # On older Rubies, we use the debase-ruby_core_source gem to get access to private VM headers.
+  # This gem ships source code copies of these VM headers for the different Ruby VM versions;
+  # see https://github.com/ruby-debug/debase-ruby_core_source for details
+
+  thread_native_for_ruby_2_1 = proc { true }
+  if RUBY_VERSION < '2.2'
+    # This header became public in Ruby 2.2, but we need to pull it from the private headers folder for 2.1
+    thread_native_for_ruby_2_1 = proc { have_header('thread_native.h') }
+    $defs << '-DRUBY_2_1_WORKAROUND'
+  end
+
+  create_header
+
+  require 'debase/ruby_core_source'
+  dir_config('ruby') # allow user to pass in non-standard core include directory
+
+  Debase::RubyCoreSource
+    .create_makefile_with_core(proc { have_header('vm_core.h') && thread_native_for_ruby_2_1.call }, EXTENSION_NAME)
+end

ext/ddtrace_profiling_native_extension/private_vm_api_access.c

@@ -2,9 +2,19 @@
 
 // This file exports functions used to access private Ruby VM APIs and internals.
 // To do this, it imports a few VM internal (private) headers.
-// Be very careful when changing things here :)
+//
+// **Important Note**: Our medium/long-term plan is to stop relying on all private Ruby headers, and instead request and
+// contribute upstream changes so that they become official public VM APIs.
+//
+// In the meanwhile, be very careful when changing things here :)
+
 #ifdef USE_MJIT_HEADER
+// Pick up internal structures from the private Ruby MJIT header file
 #include RUBY_MJIT_HEADER
+#else
+// On older Rubies, use a copy of the VM internal headers shipped in the debase-ruby_core_source gem
+#include <vm_core.h>
+#endif
 
 // MRI has a similar rb_thread_ptr() function which we can't call it directly
 // because Ruby does not expose the thread_data_type publicly.
@@ -23,4 +33,3 @@ static inline struct rb_thread_struct *thread_struct_from_object(VALUE thread) {
 rb_nativethread_id_t pthread_id_for(VALUE thread) {
   return thread_struct_from_object(thread)->thread_id;
 }
-#endif

lib/ddtrace/profiling.rb

@@ -128,9 +128,7 @@ def self.unsupported_reason
     private_class_method def self.load_profiling
       return false unless supported?
 
-      require 'ddtrace/profiling/ext/cpu'
       require 'ddtrace/profiling/ext/forking'
-
       require 'ddtrace/profiling/collectors/stack'
       require 'ddtrace/profiling/exporter'
       require 'ddtrace/profiling/recorder'