diff --git a/profiling/src/bindings/mod.rs b/profiling/src/bindings/mod.rs index ac60a6bdbc2..4de29958f5f 100644 --- a/profiling/src/bindings/mod.rs +++ b/profiling/src/bindings/mod.rs @@ -322,6 +322,10 @@ extern "C" { /// Must be called from a PHP thread during a request. pub fn datadog_php_profiling_vm_interrupt_addr() -> *const AtomicBool; + /// Returns Failure when this PHP build/runtime has the tailcall VM + /// interrupt crash that requires time sample collection to be disabled. + pub fn ddog_php_prof_check_tailcall_vm_interrupt() -> ZendResult; + /// Registers the extension. Note that it's kept in a zend_llist and gets /// pemalloc'd + memcpy'd into place. The engine says this is a mutable /// pointer, but in practice it's const. diff --git a/profiling/src/capi.rs b/profiling/src/capi.rs index 0ad6588fb3b..acae5a7daec 100644 --- a/profiling/src/capi.rs +++ b/profiling/src/capi.rs @@ -43,7 +43,10 @@ extern "C" fn ddog_php_prof_trigger_time_sample() { use std::sync::atomic::Ordering; let result = super::REQUEST_LOCALS.try_with_borrow(|locals| { - if locals.system_settings().profiling_enabled { + let system_settings = locals.system_settings(); + if system_settings.profiling_wall_time_enabled + | system_settings.profiling_experimental_cpu_time_enabled + { // Safety: only vm interrupts are stored there, or possibly null (edges only). if let Some(vm_interrupt) = unsafe { locals.vm_interrupt_addr.as_ref() } { locals.interrupt_count.fetch_add(1, Ordering::SeqCst); diff --git a/profiling/src/config.rs b/profiling/src/config.rs index bb0a44b107e..2544dfd3a6e 100644 --- a/profiling/src/config.rs +++ b/profiling/src/config.rs @@ -132,6 +132,21 @@ impl SystemSettings { let mut system_settings = SystemSettings::new(); // Work around version-specific issues. + let tailcall_check = unsafe { bindings::ddog_php_prof_check_tailcall_vm_interrupt() }; + if system_settings.profiling_enabled + && (system_settings.profiling_wall_time_enabled + | system_settings.profiling_experimental_cpu_time_enabled) + && tailcall_check != bindings::ZendResult::Success + { + error!(concat!( + "Wall- and CPU-time sample types are disabled because their VM interrupts can crash PHP 8.5.0-8.5.6 builds that have the tailcall VM.", + " To re-enable them, upgrade PHP to 8.5.7 or newer, or use a PHP build without the tailcall VM.", + " See https://github.com/php/php-src/pull/21922" + )); + system_settings.profiling_wall_time_enabled = false; + system_settings.profiling_experimental_cpu_time_enabled = false; + } + #[cfg(not(php_zend_mm_set_custom_handlers_ex))] if allocation::allocation_le83::first_rinit_should_disable_due_to_jit() { if bindings::PHP_VERSION_ID >= 80400 { @@ -1106,9 +1121,8 @@ pub(crate) fn minit(module_number: libc::c_int) { displayer: None, env_config_fallback: None, }, - // At the moment, wall-time cannot be fully disabled. This only - // controls automatic collection (manual collection is still - // possible). + // Controls wall-time collection and the wall-time related + // sample types. zai_config_entry { id: transmute::(ProfilingWallTimeEnabled), name: ProfilingWallTimeEnabled.env_var_name(), diff --git a/profiling/src/lib.rs b/profiling/src/lib.rs index ec0bd6da9f7..43aa9a941f4 100644 --- a/profiling/src/lib.rs +++ b/profiling/src/lib.rs @@ -693,7 +693,6 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { // Not logging, rinit could be quite spammy. _ = REQUEST_LOCALS.try_with_borrow(|locals| { let cpu_time_enabled = system_settings.profiling_experimental_cpu_time_enabled; - let wall_time_enabled = system_settings.profiling_wall_time_enabled; CLOCKS.with_borrow_mut(|clocks| clocks.initialize(cpu_time_enabled)); TAGS.set({ @@ -717,8 +716,8 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult { Arc::new(tags) }); - // Only add interrupt if cpu- or wall-time is enabled. - if !(cpu_time_enabled | wall_time_enabled) { + // Only add interrupt if cpu- or wall-time sample collection is enabled. + if !(cpu_time_enabled | system_settings.profiling_wall_time_enabled) { return; } @@ -775,10 +774,9 @@ extern "C" fn rshutdown(_type: c_int, _module_number: c_int) -> ZendResult { _ = REQUEST_LOCALS.try_with_borrow(|locals| { let system_settings = locals.system_settings(); - // The interrupt is only added if CPU- or wall-time are enabled BUT - // wall-time is not expected to ever be disabled, except in testing, - // and we don't need to optimize for that. - if system_settings.profiling_enabled { + if system_settings.profiling_wall_time_enabled + | system_settings.profiling_experimental_cpu_time_enabled + { if let Some(profiler) = Profiler::get() { let interrupt = VmInterrupt { interrupt_count_ptr: &locals.interrupt_count, diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index 00f684e9534..5d6782978e1 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -5,6 +5,7 @@ #include #include #include +#include #include "SAPI.h" #if CFG_STACK_WALKING_TESTS @@ -88,6 +89,18 @@ sapi_request_info datadog_sapi_globals_request_info() { */ uint32_t ddog_php_prof_php_version_id(void) { return php_version_id(); } +zend_result ddog_php_prof_check_tailcall_vm_interrupt(void) { +#if PHP_VERSION_ID >= 80500 && PHP_VERSION_ID < 80600 + uint32_t version_id = php_version_id(); + // See this PR for more information on the tailcall VM crash: + // https://github.com/php/php-src/pull/21922 + if (zend_vm_kind() == ZEND_VM_KIND_TAILCALL && version_id < 80507) { + return FAILURE; + } +#endif + return SUCCESS; +} + /** * Returns the PHP_VERSION of the engine at run-time, not the version the * extension was built against at compile-time. diff --git a/profiling/src/php_ffi.h b/profiling/src/php_ffi.h index f9aef1a21ec..1c2fd00be27 100644 --- a/profiling/src/php_ffi.h +++ b/profiling/src/php_ffi.h @@ -72,6 +72,12 @@ zend_module_entry *datadog_get_module_entry(const char *str, uintptr_t len); */ void *datadog_php_profiling_vm_interrupt_addr(void); +/** + * Detects tailcall VM interrupt bugs where time sample collection must be + * disabled. + */ +zend_result ddog_php_prof_check_tailcall_vm_interrupt(void); + /** * For Code Hotspots, we need the tracer's local root span id and the current * span id. This is a cross-product struct, so keep it in sync with tracer's diff --git a/profiling/src/profiling/sample_type_filter.rs b/profiling/src/profiling/sample_type_filter.rs index 7d6f609d576..22f242ba800 100644 --- a/profiling/src/profiling/sample_type_filter.rs +++ b/profiling/src/profiling/sample_type_filter.rs @@ -41,12 +41,17 @@ impl SampleTypeFilter { let mut sample_types_mask = [false; MAX_SAMPLE_TYPES]; if system_settings.profiling_enabled { - // sample, wall-time, cpu-time - let len = 2 + system_settings.profiling_experimental_cpu_time_enabled as usize; - sample_types.extend_from_slice(&SAMPLE_TYPES[0..len]); - sample_types_mask[0] = true; - sample_types_mask[1] = true; - sample_types_mask[2] = system_settings.profiling_experimental_cpu_time_enabled; + if system_settings.profiling_wall_time_enabled { + // sample, wall-time + sample_types.extend_from_slice(&SAMPLE_TYPES[0..2]); + sample_types_mask[0] = true; + sample_types_mask[1] = true; + } + + if system_settings.profiling_experimental_cpu_time_enabled { + sample_types.push(SAMPLE_TYPES[2]); + sample_types_mask[2] = true; + } // alloc-samples, alloc-size if system_settings.profiling_allocation_enabled { @@ -219,6 +224,36 @@ mod tests { assert_eq!(values, vec![10, 20, 30]); } + #[test] + fn filter_without_wall_or_cpu_time() { + let mut settings = get_system_settings(); + settings.profiling_enabled = true; + settings.profiling_wall_time_enabled = false; + settings.profiling_experimental_cpu_time_enabled = false; + + let sample_type_filter = SampleTypeFilter::new(&settings); + let values = sample_type_filter.filter(get_samples()); + let types = sample_type_filter.sample_types(); + + assert_eq!(types, Vec::::new()); + assert_eq!(values, Vec::::new()); + } + + #[test] + fn filter_with_cpu_time_and_without_wall_time() { + let mut settings = get_system_settings(); + settings.profiling_enabled = true; + settings.profiling_wall_time_enabled = false; + settings.profiling_experimental_cpu_time_enabled = true; + + let sample_type_filter = SampleTypeFilter::new(&settings); + let values = sample_type_filter.filter(get_samples()); + let types = sample_type_filter.sample_types(); + + assert_eq!(types, vec![ValueType::new("cpu-time", "nanoseconds")]); + assert_eq!(values, vec![30]); + } + #[test] fn filter_with_allocations() { let mut settings = get_system_settings(); @@ -242,6 +277,28 @@ mod tests { assert_eq!(values, vec![10, 20, 40, 50]); } + #[test] + fn filter_with_allocations_without_wall_or_cpu_time() { + let mut settings = get_system_settings(); + settings.profiling_enabled = true; + settings.profiling_allocation_enabled = true; + settings.profiling_wall_time_enabled = false; + settings.profiling_experimental_cpu_time_enabled = false; + + let sample_type_filter = SampleTypeFilter::new(&settings); + let values = sample_type_filter.filter(get_samples()); + let types = sample_type_filter.sample_types(); + + assert_eq!( + types, + vec![ + ValueType::new("alloc-samples", "count"), + ValueType::new("alloc-size", "bytes"), + ] + ); + assert_eq!(values, vec![40, 50]); + } + #[test] fn filter_with_allocations_and_cpu_time() { let mut settings = get_system_settings(); diff --git a/profiling/src/wall_time.rs b/profiling/src/wall_time.rs index bfd9390c90d..b2cf01dbe3d 100644 --- a/profiling/src/wall_time.rs +++ b/profiling/src/wall_time.rs @@ -110,7 +110,10 @@ static mut PREV_INTERRUPT_FUNCTION: Option = None; #[inline(never)] pub extern "C" fn ddog_php_prof_interrupt_function(execute_data: *mut zend_execute_data) { let result = REQUEST_LOCALS.try_with_borrow(|locals| { - if !locals.system_settings().profiling_enabled { + let system_settings = locals.system_settings(); + if !(system_settings.profiling_wall_time_enabled + | system_settings.profiling_experimental_cpu_time_enabled) + { return; } diff --git a/profiling/tests/phpt/phpinfo_07.phpt b/profiling/tests/phpt/phpinfo_07.phpt index 526777e724b..6810097fe8d 100644 --- a/profiling/tests/phpt/phpinfo_07.phpt +++ b/profiling/tests/phpt/phpinfo_07.phpt @@ -40,11 +40,21 @@ foreach ($lines as $line) { // Check that Version exists, but not its value assert(isset($values["Version"])); -// Check exact values for this set +$tailcall_vm_interrupt_workaround_applies = + PHP_VERSION_ID >= 80500 && + PHP_VERSION_ID < 80507 && + defined('ZEND_VM_KIND') && + ZEND_VM_KIND === 'ZEND_VM_KIND_TAILCALL'; + +$cpu_time_expected = $tailcall_vm_interrupt_workaround_applies + ? "false" + : "true (all experimental features enabled)"; + +// Check exact values for this set. $sections = [ ["Profiling Enabled", "true"], ["Profiling Experimental Features Enabled", "true"], - ["Experimental CPU Time Profiling Enabled", "true (all experimental features enabled)"], + ["Experimental CPU Time Profiling Enabled", $cpu_time_expected], ["Allocation Profiling Enabled", "false"], ["Exception Profiling Enabled", "false"], ["Timeline Enabled", "false"], @@ -52,9 +62,10 @@ $sections = [ ]; foreach ($sections as [$key, $expected]) { + $actual = $values[$key] ?? null; assert( - $values[$key] === $expected, - "Expected '{$expected}', found '{$values[$key]}', for key '{$key}'" + $actual === $expected, + "Expected '{$expected}', found '{$actual}', for key '{$key}'" ); } @@ -62,4 +73,4 @@ echo "Done."; ?> --EXPECT-- -Done. \ No newline at end of file +Done.