ci(profiling): run clippy, fix lints (#2635)

* lint(profiling): fix clippy::useless_conversion

* lint(profiling): avoid creating a shared reference to mutable static

* lint(profiling): fix unused func in stack walking test

* lint(profiling): fix clippy::large_enum_variant

* ci(profiling): run clippy for NTS builds

* lint(profiling): fix clippy::result_large_err

* ci(profiling): clippy only on x86_64-unknown-linux-gnu

* lint(profiling): unused-unsafe

* lint(profiling): fix unused StringTable

* lint(profiling): fix -Wunused-parameter for PHP 7

* lint(profiling): fix unused imports for PHP 7

.circleci/continue_config.yml

@@ -2927,6 +2927,20 @@ jobs:
             export TEST_PHP_EXECUTABLE=$(which php)
             php run-tests.php -d "extension=/mnt/ramdisk/cargo/release/libdatadog_php_profiling.so" --show-diff -g "FAIL,XFAIL,BORK,WARN,LEAK,XLEAK,SKIP" "phpt"
 
+      - run:
+          name: clippy NTS (for select platforms)
+          command: |
+            if [[ "x86_64-unknown-linux-gnu" != "<< parameters.triplet >>" ]] ; then
+              exit 0
+            fi
+            set -u
+            command -v switch-php && switch-php "${PHP_VERSION}"
+            set -e
+            cd profiling
+            touch build.rs
+            sed -i -e "s/crate-type.*$/crate-type = [\"rlib\"]/g" Cargo.toml
+            cargo clippy --all-targets --all-features -- -D warnings -Aunknown-lints
+
   compile_extension_centos:
     working_directory: ~/datadog
     parameters:

profiling/benches/stack_walking.rs

@@ -30,7 +30,7 @@ fn benchmark_instructions(c: &mut Criterion<Perf>) {
         let stack = unsafe { zend::ddog_php_test_create_fake_zend_execute_data(99) };
         group.throughput(criterion::Throughput::Elements(*depth as u64));
         group.bench_with_input(BenchmarkId::from_parameter(depth), depth, |b, &_depth| {
-            b.iter(|| unsafe { collect_stack_sample(black_box(stack)) })
+            b.iter(|| collect_stack_sample(black_box(stack)))
         });
     }
     group.finish();

profiling/build.rs

@@ -143,7 +143,7 @@ fn build_zend_php_ffis(
     let stack_walking_tests = "0";
 
     cc::Build::new()
-        .files(files.into_iter().chain(zai_c_files.into_iter()))
+        .files(files.into_iter().chain(zai_c_files))
         .define("CFG_POST_STARTUP_CB", post_startup_cb)
         .define("CFG_PRELOAD", preload)
         .define("CFG_FIBERS", fibers)

profiling/src/allocation.rs

@@ -317,7 +317,7 @@ pub fn alloc_prof_startup() {
     unsafe {
         let handle = datadog_php_zif_handler::new(
             ffi::CStr::from_bytes_with_nul_unchecked(b"gc_mem_caches\0"),
-            &mut GC_MEM_CACHES_HANDLER,
+            ptr::addr_of_mut!(GC_MEM_CACHES_HANDLER),
             Some(alloc_prof_gc_mem_caches),
         );
         datadog_php_install_handler(handle);

profiling/src/bindings/mod.rs

@@ -378,7 +378,7 @@ pub type InternalFunctionHandler =
 impl datadog_php_zif_handler {
     pub fn new(
         name: &'static CStr,
-        old_handler: &'static mut InternalFunctionHandler,
+        old_handler: *mut InternalFunctionHandler,
         new_handler: InternalFunctionHandler,
     ) -> Self {
         let name = name.to_bytes();

profiling/src/lib.rs

@@ -6,6 +6,8 @@ mod logging;
 mod pcntl;
 pub mod profiling;
 mod sapi;
+
+#[cfg(php_run_time_cache)]
 mod string_table;
 
 #[cfg(feature = "allocation_profiling")]
@@ -166,8 +168,8 @@ pub extern "C" fn get_module() -> &'static mut zend::ModuleEntry {
         ..Default::default()
     });
 
-    // SAFETY: well, it's as least as safe as what every single C extension does.
-    unsafe { &mut MODULE }
+    // SAFETY: well, it's at least as safe as what every single C extension does.
+    unsafe { &mut *ptr::addr_of_mut!(MODULE) }
 }
 
 /* Important note on the PHP lifecycle:
@@ -398,7 +400,8 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
     trace!("RINIT({_type}, {_module_number})");
 
     // SAFETY: not being mutated during rinit.
-    unsafe { &ZAI_CONFIG_ONCE }.call_once(|| unsafe {
+    let once = unsafe { &*ptr::addr_of!(ZAI_CONFIG_ONCE) };
+    once.call_once(|| unsafe {
         bindings::zai_config_first_time_rinit(true);
         config::first_rinit();
     });
@@ -436,12 +439,14 @@ extern "C" fn rinit(_type: c_int, _module_number: c_int) -> ZendResult {
     // SAFETY: still safe to access in rinit after first_rinit.
     let system_settings = unsafe { system_settings.as_ref() };
 
-    unsafe { &RINIT_ONCE }.call_once(|| {
+    // SAFETY: the once control is not mutable during request.
+    let once = unsafe { &*ptr::addr_of!(RINIT_ONCE) };
+    once.call_once(|| {
         if system_settings.profiling_enabled {
             /* Safety: sapi_module is initialized by rinit and shouldn't be
              * modified at this point (safe to read values).
              */
-            let sapi_module = unsafe { &zend::sapi_module };
+            let sapi_module = unsafe { &*ptr::addr_of!(zend::sapi_module) };
             if sapi_module.pretty_name.is_null() {
                 // Safety: I'm willing to bet the module name is less than `isize::MAX`.
                 let name = unsafe { CStr::from_ptr(sapi_module.name) }.to_string_lossy();

profiling/src/pcntl.rs

@@ -7,6 +7,7 @@ use crate::{config, Profiler, PROFILER};
 use log::{error, warn};
 use std::ffi::CStr;
 use std::mem::{forget, swap};
+use std::ptr;
 
 static mut PCNTL_FORK_HANDLER: InternalFunctionHandler = None;
 static mut PCNTL_RFORK_HANDLER: InternalFunctionHandler = None;
@@ -136,9 +137,21 @@ pub(crate) unsafe fn startup() {
      * Safety: we can modify our own globals in the startup context.
      */
     let handlers = [
-        datadog_php_zif_handler::new(PCNTL_FORK, &mut PCNTL_FORK_HANDLER, Some(pcntl_fork)),
-        datadog_php_zif_handler::new(PCNTL_RFORK, &mut PCNTL_RFORK_HANDLER, Some(pcntl_rfork)),
-        datadog_php_zif_handler::new(PCNTL_FORKX, &mut PCNTL_FORKX_HANDLER, Some(pcntl_forkx)),
+        datadog_php_zif_handler::new(
+            PCNTL_FORK,
+            ptr::addr_of_mut!(PCNTL_FORK_HANDLER),
+            Some(pcntl_fork),
+        ),
+        datadog_php_zif_handler::new(
+            PCNTL_RFORK,
+            ptr::addr_of_mut!(PCNTL_RFORK_HANDLER),
+            Some(pcntl_rfork),
+        ),
+        datadog_php_zif_handler::new(
+            PCNTL_FORKX,
+            ptr::addr_of_mut!(PCNTL_FORKX_HANDLER),
+            Some(pcntl_forkx),
+        ),
     ];
 
     for handler in handlers.into_iter() {

profiling/src/php_ffi.c

@@ -319,7 +319,8 @@ void ddog_php_prof_function_run_time_cache_init(const char *module_name) {
      */
 }
 
-#if CFG_RUN_TIME_CACHE // defined by build.rs
+// defined by build.rs
+#if CFG_RUN_TIME_CACHE && !CFG_STACK_WALKING_TESTS
 static bool has_invalid_run_time_cache(zend_function const *func) {
     if (UNEXPECTED(_ignore_run_time_cache) || UNEXPECTED(ddog_php_prof_run_time_cache_handle < 0))
         return true;
@@ -398,6 +399,7 @@ uintptr_t *ddog_test_php_prof_function_run_time_cache(zend_function const *func)
     return *non_const_func->common.run_time_cache__ptr;
 #endif
 #else
+    (void)func;
     return NULL;
 #endif
 }

profiling/src/profiling/mod.rs

@@ -246,12 +246,12 @@ impl TimeCollector {
         let end_time = wall_export.systemtime;
 
         for (index, profile) in profiles.drain() {
-            let message = UploadMessage::Upload(UploadRequest {
+            let message = UploadMessage::Upload(Box::new(UploadRequest {
                 index,
                 profile,
                 end_time,
                 duration,
-            });
+            }));
             if let Err(err) = self.upload_sender.try_send(message) {
                 warn!("Failed to upload profile: {err}");
             }
@@ -505,7 +505,7 @@ pub struct UploadRequest {
 
 pub enum UploadMessage {
     Pause,
-    Upload(UploadRequest),
+    Upload(Box<UploadRequest>),
 }
 
 impl Profiler {
@@ -597,17 +597,22 @@ impl Profiler {
         self.fork_barrier.wait();
     }
 
-    pub fn send_sample(&self, message: SampleMessage) -> Result<(), TrySendError<ProfilerMessage>> {
+    pub fn send_sample(
+        &self,
+        message: SampleMessage,
+    ) -> Result<(), Box<TrySendError<ProfilerMessage>>> {
         self.message_sender
             .try_send(ProfilerMessage::Sample(message))
+            .map_err(Box::new)
     }
 
     pub fn send_local_root_span_resource(
         &self,
         message: LocalRootSpanResourceMessage,
-    ) -> Result<(), TrySendError<ProfilerMessage>> {
+    ) -> Result<(), Box<TrySendError<ProfilerMessage>>> {
         self.message_sender
             .try_send(ProfilerMessage::LocalRootSpanResource(message))
+            .map_err(Box::new)
     }
 
     /// Begins the shutdown process. To complete it, call [Profiler::shutdown].

profiling/src/profiling/stalk_walking.rs

@@ -1,9 +1,12 @@
 use crate::bindings::{zai_str_from_zstr, zend_execute_data, zend_function, ZEND_USER_FUNCTION};
-use crate::string_table::StringTable;
 use std::borrow::Cow;
-use std::cell::RefCell;
 use std::str::Utf8Error;
 
+#[cfg(php_run_time_cache)]
+use crate::string_table::StringTable;
+#[cfg(php_run_time_cache)]
+use std::cell::RefCell;
+
 /// Used to help track the function run_time_cache hit rate. It glosses over
 /// the fact that there are two cache slots used, and they don't have to be in
 /// sync. However, they usually are, so we simplify.
@@ -296,13 +299,12 @@ pub fn collect_stack_sample(
     }
 }
 
-#[cfg(test)]
+#[cfg(all(test, stack_walking_tests))]
 mod tests {
     use super::*;
     use crate::bindings as zend;
 
     #[test]
-    #[cfg(feature = "stack_walking_tests")]
     fn test_collect_stack_sample() {
         unsafe {
             let fake_execute_data = zend::ddog_php_test_create_fake_zend_execute_data(3);

profiling/src/profiling/uploader.rs

@@ -47,7 +47,7 @@ impl Uploader {
         Some(metadata)
     }
 
-    fn upload(&self, message: UploadRequest) -> anyhow::Result<u16> {
+    fn upload(&self, message: Box<UploadRequest>) -> anyhow::Result<u16> {
         let index = message.index;
         let profile = message.profile;
 

profiling/src/timeline.rs

@@ -7,7 +7,6 @@ use libc::c_char;
 use log::{error, trace, warn};
 use std::cell::RefCell;
 use std::ffi::CStr;
-use std::mem::MaybeUninit;
 use std::ptr;
 use std::time::Instant;
 use std::time::SystemTime;
@@ -184,27 +183,27 @@ pub unsafe fn timeline_startup() {
     let handlers = [
         zend::datadog_php_zif_handler::new(
             CStr::from_bytes_with_nul_unchecked(b"sleep\0"),
-            &mut SLEEP_HANDLER,
+            ptr::addr_of_mut!(SLEEP_HANDLER),
             Some(ddog_php_prof_sleep),
         ),
         zend::datadog_php_zif_handler::new(
             CStr::from_bytes_with_nul_unchecked(b"usleep\0"),
-            &mut USLEEP_HANDLER,
+            ptr::addr_of_mut!(USLEEP_HANDLER),
             Some(ddog_php_prof_usleep),
         ),
         zend::datadog_php_zif_handler::new(
             CStr::from_bytes_with_nul_unchecked(b"time_nanosleep\0"),
-            &mut TIME_NANOSLEEP_HANDLER,
+            ptr::addr_of_mut!(TIME_NANOSLEEP_HANDLER),
             Some(ddog_php_prof_time_nanosleep),
         ),
         zend::datadog_php_zif_handler::new(
             CStr::from_bytes_with_nul_unchecked(b"time_sleep_until\0"),
-            &mut TIME_SLEEP_UNTIL_HANDLER,
+            ptr::addr_of_mut!(TIME_SLEEP_UNTIL_HANDLER),
             Some(ddog_php_prof_time_sleep_until),
         ),
         zend::datadog_php_zif_handler::new(
             CStr::from_bytes_with_nul_unchecked(b"frankenphp_handle_request\0"),
-            &mut FRANKENPHP_HANDLE_REQUEST_HANDLER,
+            ptr::addr_of_mut!(FRANKENPHP_HANDLE_REQUEST_HANDLER),
             Some(ddog_php_prof_frankenphp_handle_request),
         ),
     ];
@@ -471,7 +470,7 @@ unsafe extern "C" fn ddog_php_prof_gc_collect_cycles() -> i32 {
         }
 
         #[cfg(php_gc_status)]
-        let mut status = MaybeUninit::<zend::zend_gc_status>::uninit();
+        let mut status = core::mem::MaybeUninit::<zend::zend_gc_status>::uninit();
 
         let start = Instant::now();
         let collected = prev();