diff --git a/.github/workflows/prof_correctness.yml b/.github/workflows/prof_correctness.yml index ce19d9b6324..fd8e449d6da 100644 --- a/.github/workflows/prof_correctness.yml +++ b/.github/workflows/prof_correctness.yml @@ -80,7 +80,7 @@ jobs: export DD_PROFILING_EXCEPTION_MESSAGE_ENABLED=1 php -v php -d extension=target/profiler-release/libdatadog_php_profiling.so --ri datadog-profiling - for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined"; do + for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined" "generators"; do mkdir -p profiling/tests/correctness/"$test_case"/ export DD_PROFILING_OUTPUT_PPROF=$PWD/profiling/tests/correctness/"$test_case"/test.pprof php -d extension="${PWD}/target/profiler-release/libdatadog_php_profiling.so" "profiling/tests/correctness/${test_case}.php" @@ -98,7 +98,7 @@ jobs: export DD_PROFILING_EXCEPTION_MESSAGE_ENABLED=1 php -v php -d extension=target/profiler-release/libdatadog_php_profiling.so --ri datadog-profiling - for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined"; do + for test_case in "allocations" "time" "strange_frames" "timeline" "exceptions" "io" "allocation_time_combined" "generators"; do mkdir -p profiling/tests/correctness/"$test_case"/ export DD_PROFILING_OUTPUT_PPROF=$PWD/profiling/tests/correctness/"$test_case"/test.pprof php -d extension=$PWD/target/profiler-release/libdatadog_php_profiling.so profiling/tests/correctness/"$test_case".php @@ -170,6 +170,12 @@ jobs: expected_json: profiling/tests/correctness/allocation_time_combined.json pprof_path: profiling/tests/correctness/allocation_time_combined/ + - name: Check profiler correctness for generators + uses: Datadog/prof-correctness/analyze@main + with: + expected_json: profiling/tests/correctness/generators.json + pprof_path: profiling/tests/correctness/generators/ + - name: Check profiler correctness for IO uses: Datadog/prof-correctness/analyze@main with: diff --git a/profiling/build.rs b/profiling/build.rs index 15e9ecb6ce2..491366c824e 100644 --- a/profiling/build.rs +++ b/profiling/build.rs @@ -542,6 +542,7 @@ fn apple_linker_flags() { "_zend_extensions", "_zend_flf_functions", "_zend_gc_get_status", + "_zend_generator_check_placeholder_frame", "_zend_get_executed_filename_ex", "_zend_get_executed_lineno", "_zend_get_extension", diff --git a/profiling/src/bindings/mod.rs b/profiling/src/bindings/mod.rs index 681777440aa..ac60a6bdbc2 100644 --- a/profiling/src/bindings/mod.rs +++ b/profiling/src/bindings/mod.rs @@ -365,6 +365,13 @@ extern "C" { func: &zend_function, ) -> Option<&mut [usize; 2]>; + /// mock for testing; passthrough stub for zend_generator_check_placeholder_frame + /// so `cargo test` builds don't need PHP's Zend symbols at link time. + #[cfg(feature = "stack_walking_tests")] + pub fn ddog_test_zend_generator_check_placeholder_frame( + ptr: *mut zend_execute_data, + ) -> *mut zend_execute_data; + /// Returns the PHP_VERSION_ID of the engine at run-time, not the version /// the extension was built against at compile-time. pub fn ddog_php_prof_php_version_id() -> u32; diff --git a/profiling/src/php_ffi.c b/profiling/src/php_ffi.c index f4b42bcdaf3..00f684e9534 100644 --- a/profiling/src/php_ffi.c +++ b/profiling/src/php_ffi.c @@ -383,6 +383,11 @@ uintptr_t *ddog_php_prof_function_run_time_cache(zend_function const *func) { } #if CFG_STACK_WALKING_TESTS +zend_execute_data *ddog_test_zend_generator_check_placeholder_frame(zend_execute_data *ptr) { + // Tests do not construct real generator placeholder frames; pass through. + return ptr; +} + uintptr_t *ddog_test_php_prof_function_run_time_cache(zend_function const *func) { #if CFG_RUN_TIME_CACHE if (_ignore_run_time_cache) return NULL; diff --git a/profiling/src/profiling/stack_walking.rs b/profiling/src/profiling/stack_walking.rs index 98642209b62..199c7796ffb 100644 --- a/profiling/src/profiling/stack_walking.rs +++ b/profiling/src/profiling/stack_walking.rs @@ -300,6 +300,11 @@ mod detail { top_execute_data: *mut zend_execute_data, string_set: &mut StringSet, ) -> Result { + #[cfg(feature = "stack_walking_tests")] + use crate::bindings::ddog_test_zend_generator_check_placeholder_frame as zend_generator_check_placeholder_frame; + #[cfg(not(feature = "stack_walking_tests"))] + use crate::bindings::zend_generator_check_placeholder_frame; + let max_depth = 512; let mut samples = Vec::new(); let mut execute_data_ptr = top_execute_data; @@ -307,6 +312,17 @@ mod detail { samples.try_reserve(max_depth >> 3)?; while let Some(execute_data) = unsafe { execute_data_ptr.as_ref() } { + // Nested generators leave a placeholder frame (func == NULL, + // This == generator object) on the stack. Resolve it to the real + // frame the same way zend_fetch_debug_backtrace and the observer + // API do. + if execute_data.func.is_null() { + execute_data_ptr = + unsafe { zend_generator_check_placeholder_frame(execute_data_ptr) }; + } + let Some(execute_data) = (unsafe { execute_data_ptr.as_ref() }) else { + break; + }; // allowed because it's only used on the frameless path #[allow(unused_variables)] if let Some(func) = unsafe { execute_data.func.as_ref() } { @@ -485,6 +501,11 @@ mod detail { pub fn collect_stack_sample( top_execute_data: *mut zend_execute_data, ) -> Result { + #[cfg(feature = "stack_walking_tests")] + use crate::bindings::ddog_test_zend_generator_check_placeholder_frame as zend_generator_check_placeholder_frame; + #[cfg(not(feature = "stack_walking_tests"))] + use crate::bindings::zend_generator_check_placeholder_frame; + #[cfg(feature = "tracing")] let _span = tracing::trace_span!("collect_stack_sample").entered(); @@ -493,6 +514,13 @@ mod detail { let mut execute_data_ptr = top_execute_data; while let Some(execute_data) = unsafe { execute_data_ptr.as_ref() } { + if execute_data.func.is_null() { + execute_data_ptr = + unsafe { zend_generator_check_placeholder_frame(execute_data_ptr) }; + } + let Some(execute_data) = (unsafe { execute_data_ptr.as_ref() }) else { + break; + }; let maybe_frame = unsafe { collect_call_frame(execute_data) }; if let Some(frame) = maybe_frame { samples.try_push(frame)?; diff --git a/profiling/tests/correctness/generators.json b/profiling/tests/correctness/generators.json new file mode 100644 index 00000000000..8c66ecbae5b --- /dev/null +++ b/profiling/tests/correctness/generators.json @@ -0,0 +1,36 @@ +{ + "scale_by_duration": true, + "test_name": "php_generators", + "stacks": [ + { + "profile-type": "alloc-size", + "stack-content": [ + { + "regular_expression": " 0.0) { + usleep((int) ($sleep * 1_000_000)); + } + } +} +main();