diff --git a/.github/workflows/prof_asan.yml b/.github/workflows/prof_asan.yml index 83e1993c31..28f8752bad 100644 --- a/.github/workflows/prof_asan.yml +++ b/.github/workflows/prof_asan.yml @@ -62,7 +62,7 @@ jobs: export DDTRACE_PROFILING_TARGET="$(uname -m)-unknown-linux-gnu" export DDTRACE_PROFILING_CARGO_BUILD_FLAGS='-Zbuild-std=std,panic_abort' phpize - ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling --disable-ddtrace-rust-debug + DDTRACE_PROFILING_FEATURES="debug_stats,test,tracing,tracing-subscriber,trigger_time_sample" ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling --disable-ddtrace-rust-debug make -j"$(nproc)" cp -v modules/datadog-profiling.so "$(php-config --extension-dir)/datadog-profiling.so" @@ -128,7 +128,7 @@ jobs: export RUSTC_LINKER=clang-21 export RUSTFLAGS='-C link-arg=-fsanitize=undefined,local-bounds -C link-arg=-fno-sanitize-recover=all' phpize - ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling + DDTRACE_PROFILING_FEATURES="debug_stats,test,tracing,tracing-subscriber,trigger_time_sample" ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling make -j"$(nproc)" cp -v modules/datadog-profiling.so "$(php-config --extension-dir)/datadog-profiling.so" diff --git a/.gitlab/generate-profiler.php b/.gitlab/generate-profiler.php index 366dac61da..c53a7a1bd0 100644 --- a/.gitlab/generate-profiler.php +++ b/.gitlab/generate-profiler.php @@ -59,14 +59,14 @@ - '# NTS' - '# Use if/then instead of `command -v switch-php && switch-php` — the && form exits 1 when switch-php is absent, which FF_ENABLE_BASH_EXIT_CODE_CHECK treats as a job failure' - if command -v switch-php > /dev/null 2>&1; then switch-php "${PHP_MAJOR_MINOR}"; fi - - (cd ..; phpize && DDTRACE_PROFILING_FEATURES="debug_stats,stack_walking_tests,test,tracing,tracing-subscriber,trigger_time_sample" ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling && make -j$(nproc)) + - (cd ..; phpize && DDTRACE_PROFILING_FEATURES="debug_stats,test,tracing,tracing-subscriber,trigger_time_sample" ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling && make -j$(nproc)) - (cd ../; TEST_PHP_JUNIT="${CI_PROJECT_DIR}/artifacts/profiler-tests/nts-results.xml" php profiling/tests/run-tests.php -d "extension=${CI_PROJECT_DIR}/modules/datadog-profiling.so" --show-diff -g "FAIL,XFAIL,BORK,WARN,LEAK,XLEAK,SKIP" "profiling/tests/phpt") - '# ZTS' - if command -v switch-php > /dev/null 2>&1; then switch-php "${PHP_MAJOR_MINOR}-zts"; fi - touch ../profiling/build.rs # force regeneration after switch-php changes the php-config symlink target - - (cd ..; make distclean || true; phpize && DDTRACE_PROFILING_FEATURES="debug_stats,stack_walking_tests,test,tracing,tracing-subscriber,trigger_time_sample" ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling && make -j$(nproc)) + - (cd ..; make distclean || true; phpize && DDTRACE_PROFILING_FEATURES="debug_stats,test,tracing,tracing-subscriber,trigger_time_sample" ./configure --disable-ddtrace-tracer --enable-ddtrace-profiling && make -j$(nproc)) - (cd ../; TEST_PHP_JUNIT="${CI_PROJECT_DIR}/artifacts/profiler-tests/zts-results.xml" php profiling/tests/run-tests.php -d "extension=${CI_PROJECT_DIR}/modules/datadog-profiling.so" --show-diff -g "FAIL,XFAIL,BORK,WARN,LEAK,XLEAK,SKIP" "profiling/tests/phpt") after_script: - | diff --git a/profiling/build.rs b/profiling/build.rs index 2a04ae599e..874ca5fc0e 100644 --- a/profiling/build.rs +++ b/profiling/build.rs @@ -575,11 +575,14 @@ fn apple_linker_flags() { "_zend_accel_schedule_restart_hook", "_zend_alter_ini_entry_ex", "_zend_ce_throwable", + "_zend_call_function", + "_zend_call_stack_init", "_zend_compile_file", "_zend_compile_string", "_zend_empty_string", "_zend_extensions", "_zend_flf_functions", + "_zend_fcall_info_init", "_zend_flf_handlers", "_zend_gc_get_status", "_zend_generator_check_placeholder_frame", @@ -613,6 +616,7 @@ fn apple_linker_flags() { "_zend_mm_shutdown", "_zend_new_interned_string", "_zend_observer_error_register", + "_zend_parse_parameters", "_zend_post_startup_cb", "_zend_register_extension", "_zend_register_ini_entries", @@ -652,6 +656,8 @@ fn apple_linker_flags() { "_executor_globals_offset", "_sapi_globals_offset", // Zend parameter error (may appear with certain PHP versions/features) + "_zend_wrong_parameter_error", + "_zend_wrong_parameters_count_error", "_zend_wrong_parameters_none_error", ]; diff --git a/profiling/src/config.rs b/profiling/src/config.rs index 4f582fc394..78ef94479a 100644 --- a/profiling/src/config.rs +++ b/profiling/src/config.rs @@ -151,6 +151,16 @@ impl SystemSettings { system_settings.profiling_experimental_heap_live_enabled = false; } + // ext-grpc can run PHP on a native thread while the main thread does I/O. + // Sampling that I/O would race with changes to the shared NTS PHP stack. + #[cfg(feature = "io_profiling")] + if system_settings.profiling_io_enabled + && !bindings::datadog_get_module_entry(c"grpc".as_ptr(), 4).is_null() + { + error!("I/O profiling is disabled because ext-grpc can execute PHP on native threads."); + system_settings.profiling_io_enabled = false; + } + SystemSettings::log_state( (*ptr::addr_of!(SYSTEM_SETTINGS)).state, system_settings.state, diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index c082319566..0d724cf214 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -944,6 +944,8 @@ unsafe extern "C" fn minfo(module_ptr: *mut zend::ModuleEntry) { c"I/O Profiling Enabled".as_ptr(), if system_settings.profiling_io_enabled { yes + } else if !zend::datadog_get_module_entry(c"grpc".as_ptr(), 4).is_null() { + c"false (incompatible with ext-grpc)".as_ptr() } else if system_settings.profiling_enabled { no } else { diff --git a/profiling/src/module_globals.rs b/profiling/src/module_globals.rs index f76e2aab07..56ce505aae 100644 --- a/profiling/src/module_globals.rs +++ b/profiling/src/module_globals.rs @@ -7,7 +7,9 @@ use core::sync::atomic::AtomicU32; #[cfg(target_os = "linux")] use crate::profiling::process_context::ProcessContextCache; -#[cfg(target_os = "linux")] +#[cfg(php_run_time_cache)] +use crate::profiling::string_set::StringSet; +#[cfg(any(target_os = "linux", php_run_time_cache))] use core::cell::RefCell; #[cfg(php_zend_mm_set_custom_handlers_ex)] @@ -31,6 +33,9 @@ pub struct ProfilerGlobals { /// Per-thread allocation sampling state. Kept in PHP globals so allocator /// hooks can reuse an already-resolved TSRM cache instead of accessing Rust TLS. pub allocation_profiling_stats: UnsafeCell>, + /// String cache backing pointers stored in PHP runtime cache slots. + #[cfg(php_run_time_cache)] + pub cached_strings: UnsafeCell>>, } /// We need TSRM to call into GINIT and GSHUTDOWN to observe spawning and @@ -51,6 +56,8 @@ pub static mut GLOBALS: ProfilerGlobals = ProfilerGlobals { #[cfg(target_os = "linux")] process_context: RefCell::new(ProcessContextCache::new()), allocation_profiling_stats: UnsafeCell::new(MaybeUninit::uninit()), + #[cfg(php_run_time_cache)] + cached_strings: UnsafeCell::new(MaybeUninit::uninit()), }; #[cfg(php_zts)] @@ -129,11 +136,13 @@ pub unsafe extern "C" fn ginit(_globals_ptr: *mut c_void) { #[cfg(php_zts)] crate::profiling::timeline::timeline_ginit(); - // Initialize PHP globals for ZTS builds. For NTS builds, this was already - // done in its const initializer. + #[cfg(any(php_zts, php_run_time_cache))] + let globals = _globals_ptr.cast::(); + + // Initialize PHP globals for ZTS builds. For NTS builds, the const fields + // were already initialized above. #[cfg(php_zts)] { - let globals = _globals_ptr.cast::(); (*globals).zend_mm_state = Cell::new(ZendMMState::new()); (*globals).interrupt_count = AtomicU32::new(0); #[cfg(target_os = "linux")] @@ -142,6 +151,9 @@ pub unsafe extern "C" fn ginit(_globals_ptr: *mut c_void) { (*globals).allocation_profiling_stats = UnsafeCell::new(MaybeUninit::uninit()); } + #[cfg(php_run_time_cache)] + (*(*globals).cached_strings.get()).write(RefCell::new(StringSet::new())); + // SAFETY: this is called in thread ginit as expected, and no other places. allocation::ginit(); } @@ -155,9 +167,11 @@ pub unsafe extern "C" fn gshutdown(_globals_ptr: *mut c_void) { #[cfg(php_zts)] crate::profiling::timeline::timeline_gshutdown(); + #[cfg(any(target_os = "linux", php_run_time_cache))] + let globals = _globals_ptr.cast::(); + #[cfg(target_os = "linux")] { - let globals = _globals_ptr.cast::(); if let Ok(mut cache) = (*globals).process_context.try_borrow_mut() { cache.reset(); } @@ -167,6 +181,9 @@ pub unsafe extern "C" fn gshutdown(_globals_ptr: *mut c_void) { // SAFETY: this is called in thread gshutdown as expected, no other places. allocation::gshutdown(); + + #[cfg(php_run_time_cache)] + (*(*globals).cached_strings.get()).assume_init_drop(); } // Unit tests are not loaded by PHP, so provide the PHP globals and TSRM symbol diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 3d0cc6285b..805c50b0c1 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -165,7 +165,11 @@ static bool _ignore_run_time_cache = false; void datadog_php_profiling_startup(zend_extension *extension) { #if CFG_RUN_TIME_CACHE // defined by build.rs +#ifdef CFG_TEST + _ignore_run_time_cache = false; +#else _ignore_run_time_cache = strcmp(sapi_module.name, "cli") == 0; +#endif #endif datadog_php_profiling_get_profiling_context = noop_get_profiling_context; @@ -718,38 +722,37 @@ ZEND_END_ARG_INFO() #if CFG_TEST && !defined(ZTS) #include -static void* native_thread_alloc_func(void* arg) { - (void)arg; - - // Allocate 2x default sampling distance to make sure we trigger the - // allocation profiler - void* ptr = emalloc(8 * 1024 * 1024); - if (ptr) { - efree(ptr); - } +typedef struct { + zend_fcall_info fci; + zend_fcall_info_cache fcc; +} native_thread_call; +static void* native_thread_call_func(void* arg) { + native_thread_call *call = arg; + zend_call_function(&call->fci, &call->fcc); return NULL; } -// Test function to simulate what ext-grpc does: create a native thread (not a -// PHP thread) and trigger memory allocation on it. This tests that the -// allocation profiler correctly handles allocations from non-PHP threads in NTS -// builds. This not something anyone should do, but ext-grpc does it anyway. -static ZEND_FUNCTION(Datadog_Profiling_run_alloc_on_native_thread) { - zend_parse_parameters_none(); +// Simulate ext-grpc executing PHP on a native thread in NTS. The main thread +// must wait for the callback: both threads share the same PHP globals. +static ZEND_FUNCTION(Datadog_Profiling_run_on_native_thread) { + native_thread_call call = {0}; + if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &call.fci, &call.fcc) == FAILURE) { + return; + } + call.fci.retval = return_value; pthread_t thread; - if (pthread_create(&thread, NULL, native_thread_alloc_func, NULL) != 0) { + if (pthread_create(&thread, NULL, native_thread_call_func, &call) != 0) { php_error_docref(NULL, E_WARNING, "Failed to create native thread"); RETURN_FALSE; } pthread_join(thread, NULL); - - RETURN_TRUE; } -ZEND_BEGIN_ARG_INFO_EX(arginfo_Datadog_Profiling_run_alloc_on_native_thread, 0, 0, 0) +ZEND_BEGIN_ARG_INFO_EX(arginfo_Datadog_Profiling_run_on_native_thread, 0, 0, 1) + ZEND_ARG_CALLABLE_INFO(0, callback, 0) ZEND_END_ARG_INFO() #endif @@ -765,9 +768,9 @@ static const zend_function_entry functions[] = { #if CFG_TEST && !defined(ZTS) ZEND_NS_NAMED_FE( "Datadog\\Profiling", - run_alloc_on_native_thread, - ZEND_FN(Datadog_Profiling_run_alloc_on_native_thread), - arginfo_Datadog_Profiling_run_alloc_on_native_thread + run_on_native_thread, + ZEND_FN(Datadog_Profiling_run_on_native_thread), + arginfo_Datadog_Profiling_run_on_native_thread ) #endif ZEND_FE_END diff --git a/profiling/src/profiler/stack_walking.rs b/profiling/src/profiler/stack_walking.rs index 45a94db5aa..a62fd970ee 100644 --- a/profiling/src/profiler/stack_walking.rs +++ b/profiling/src/profiler/stack_walking.rs @@ -172,10 +172,13 @@ unsafe fn extract_file_and_line( #[cfg(php_run_time_cache)] mod detail { use super::*; + use crate::profiling::module_globals; use crate::profiling::string_set::StringSet; - use crate::profiling::{RefCellExt, RefCellExtError}; + #[cfg(feature = "debug_stats")] + use crate::profiling::RefCellExt; use libdd_profiling::profiles::collections::ThinStr; use log::{debug, trace}; + #[cfg(feature = "debug_stats")] use std::cell::RefCell; use std::ffi::c_void; @@ -250,9 +253,8 @@ mod detail { } } + #[cfg(feature = "debug_stats")] thread_local! { - static CACHED_STRINGS: RefCell = RefCell::new(StringSet::new()); - #[cfg(feature = "debug_stats")] static FUNCTION_CACHE_STATS: RefCell = const { RefCell::new(FunctionRunTimeCacheStats::new()) } } @@ -274,7 +276,13 @@ mod detail { }); } - let result = CACHED_STRINGS.try_with_borrow_mut(|string_set| { + // SAFETY: RSHUTDOWN runs after GINIT and before GSHUTDOWN on the + // current PHP thread. + let cached_strings = unsafe { + let globals = module_globals::get_profiler_globals(); + (&*(*globals).cached_strings.get()).assume_init_ref() + }; + let result = cached_strings.try_borrow_mut().map(|mut string_set| { // A slow ramp up to 2 MiB is probably _not_ going to look like a // memory leak. A higher threshold may make a user suspect a leak. const THRESHOLD: usize = 2 * 1024 * 1024; @@ -293,7 +301,7 @@ mod detail { if let Err(err) = result { // Debug level because rshutdown could be quite spammy. - debug!("failed to borrow request locals in rshutdown: {err}"); + debug!("failed to borrow string cache in rshutdown: {err}"); } } @@ -303,7 +311,7 @@ mod detail { /// Returns [`CollectStackSampleError::TryReserveError`] if the vec holding the frames is /// unable to allocate memory. #[inline] - fn collect_stack_sample_cached( + pub(super) fn collect_stack_sample_cached( top_execute_data: *mut zend_execute_data, string_set: &mut StringSet, ) -> Result { @@ -389,13 +397,14 @@ mod detail { ) -> Result { #[cfg(feature = "tracing")] let _span = tracing::trace_span!("collect_stack_sample").entered(); - CACHED_STRINGS - .try_with_borrow_mut(|set| collect_stack_sample_cached(execute_data, set)) - .unwrap_or_else(|err| match err { - RefCellExtError::AccessError(e) => Err(e.into()), - RefCellExtError::BorrowError(e) => Err(e.into()), - RefCellExtError::BorrowMutError(e) => Err(e.into()), - }) + // SAFETY: stack samples are collected after GINIT and before + // GSHUTDOWN on the current PHP thread. + let cached_strings = unsafe { + let globals = module_globals::get_profiler_globals(); + (&*(*globals).cached_strings.get()).assume_init_ref() + }; + let mut set = cached_strings.try_borrow_mut()?; + collect_stack_sample_cached(execute_data, &mut set) } unsafe fn collect_call_frame( @@ -597,6 +606,12 @@ mod tests { unsafe { let fake_execute_data = zend::ddog_php_test_create_fake_zend_execute_data(3); + #[cfg(php_run_time_cache)] + let stack = { + let mut string_set = crate::profiling::string_set::StringSet::new(); + detail::collect_stack_sample_cached(fake_execute_data, &mut string_set).unwrap() + }; + #[cfg(not(php_run_time_cache))] let stack = collect_stack_sample(fake_execute_data).unwrap(); assert_eq!(stack.len(), 3); diff --git a/profiling/tests/phpt/grpc_io_disabled.phpt b/profiling/tests/phpt/grpc_io_disabled.phpt new file mode 100644 index 0000000000..b0f431a669 --- /dev/null +++ b/profiling/tests/phpt/grpc_io_disabled.phpt @@ -0,0 +1,40 @@ +--TEST-- +[profiling] ext-grpc disables I/O profiling even with all experimental features enabled +--DESCRIPTION-- +gRPC itself leaks on unload, even without the profiler. +ZEND_DONT_UNLOAD_MODULES=1 keeps its global references visible to LeakSanitizer. +--EXTENSIONS-- +grpc +--SKIPIF-- +info(); +$info = ob_get_clean(); +if (strpos($info, 'built without I/O profiling support') !== false) + die('skip: test requires I/O profiling support'); +?> +--ENV-- +DD_PROFILING_ENABLED=yes +DD_PROFILING_EXPERIMENTAL_FEATURES_ENABLED=yes +DD_PROFILING_EXPERIMENTAL_IO_ENABLED=no +ZEND_DONT_UNLOAD_MODULES=1 +--INI-- +datadog.profiling.log_level=error +--FILE-- +info(); +$info = ob_get_clean(); +foreach (preg_split('/\R/', $info) as $line) { + if (strpos($line, 'I/O Profiling Enabled') === 0) { + echo $line, PHP_EOL; + } +} +?> +--EXPECTF-- +%AI/O profiling is disabled because ext-grpc can execute PHP on native threads. +%AI/O Profiling Enabled => false (incompatible with ext-grpc) diff --git a/profiling/tests/phpt/native_thread_alloc_01.phpt b/profiling/tests/phpt/native_thread_alloc_01.phpt index 3404e1c445..591ff5f8cd 100644 --- a/profiling/tests/phpt/native_thread_alloc_01.phpt +++ b/profiling/tests/phpt/native_thread_alloc_01.phpt @@ -1,29 +1,43 @@ --TEST-- -[profiling] allocation profiling should not crash when allocation happens on non-PHP thread (ext-grpc compatibility) +[profiling] reuse a closure after executing it on a native thread (ext-grpc compatibility) --DESCRIPTION-- -This test simulates what ext-grpc does: it creates a native thread (not a PHP thread) and triggers memory allocation on it. Before the fix, this would crash because: -1. ThreadRng uses thread-local storage internally -2. ALLOCATION_PROFILING_STATS was thread-local -Both of these are uninitialized for non-PHP threads since they never went through GINIT. After the fix, NTS builds use a global static instead of TLS. -See https://github.com/DataDog/dd-trace-php/pull/3542 for the fix +Execute a PHP closure that allocates on a native thread, join it, then call the +same closure on the main thread. PHP runtime cache slots populated by the +background thread must remain valid after its Rust TLS has been destroyed. +Disable the stack limit because the callback does not use the main thread's stack. --SKIPIF-- --ENV-- DD_PROFILING_ENABLED=yes DD_PROFILING_ALLOCATION_ENABLED=yes DD_PROFILING_ALLOCATION_SAMPLING_DISTANCE=1 +--INI-- +zend.max_allowed_stack_size=-1 --FILE-- ---EXPECTF-- -Done. +--EXPECT-- +int(8388608) +Joined. +int(1) +int(8388608) +int(2)