Clean up comments for clarity

Author: gyuheon0h

bin_tests/src/modes/unix/test_010_runtime_callback_frame.rs

@@ -46,17 +46,13 @@ unsafe extern "C" fn test_runtime_callback_frame(
     emit_frame: unsafe extern "C" fn(*const RuntimeStackFrame),
     _emit_stacktrace_string: unsafe extern "C" fn(*const c_char),
 ) {
-    // Use static null-terminated strings to avoid allocation in signal context
-    // In a real runtime, these would come from the runtime's managed string pool
-    // Using fully qualified function names that include module/class hierarchy
     static FUNCTION_NAME_1: &[u8] = b"test_module.TestClass.runtime_function_1\0";
     static FUNCTION_NAME_2: &[u8] = b"my_package.submodule.MyModule.runtime_function_2\0";
     static FUNCTION_NAME_3: &[u8] = b"__main__.runtime_main\0";
     static FILE_NAME_1: &[u8] = b"script.py\0";
     static FILE_NAME_2: &[u8] = b"module.py\0";
     static FILE_NAME_3: &[u8] = b"main.py\0";
 
-    // Frame 1: test_module.TestClass.runtime_function_1 in script.py
     let frame1 = RuntimeStackFrame {
         function_name: FUNCTION_NAME_1.as_ptr() as *const c_char,
         file_name: FILE_NAME_1.as_ptr() as *const c_char,
@@ -65,7 +61,6 @@ unsafe extern "C" fn test_runtime_callback_frame(
     };
     emit_frame(&frame1);
 
-    // Frame 2: my_package.submodule.MyModule.runtime_function_2 in module.py
     let frame2 = RuntimeStackFrame {
         function_name: FUNCTION_NAME_2.as_ptr() as *const c_char,
         file_name: FILE_NAME_2.as_ptr() as *const c_char,
@@ -74,7 +69,6 @@ unsafe extern "C" fn test_runtime_callback_frame(
     };
     emit_frame(&frame2);
 
-    // Frame 3: __main__.runtime_main in main.py
     let frame3 = RuntimeStackFrame {
         function_name: FUNCTION_NAME_3.as_ptr() as *const c_char,
         file_name: FILE_NAME_3.as_ptr() as *const c_char,
@@ -96,10 +90,8 @@ mod tests {
             clear_runtime_callback();
         }
 
-        // Test that no callback is initially registered
         assert!(!is_runtime_callback_registered());
 
-        // Test frame mode registration
         let result =
             register_runtime_stack_callback(test_runtime_callback_frame, CallbackType::Frame);
         assert!(result.is_ok(), "Frame callback registration should succeed");
@@ -108,7 +100,6 @@ mod tests {
             "Callback should be registered"
         );
 
-        // Clean up
         unsafe {
             clear_runtime_callback();
         }

bin_tests/src/modes/unix/test_011_runtime_callback_string.rs

@@ -60,15 +60,12 @@ mod tests {
 
     #[test]
     fn test_runtime_callback_string_registration() {
-        // Ensure clean state
         unsafe {
             clear_runtime_callback();
         }
 
-        // Test that no callback is initially registered
         assert!(!is_runtime_callback_registered());
 
-        // Test string mode registration
         let result = register_runtime_stack_callback(
             test_runtime_callback_string,
             CallbackType::StacktraceString,
@@ -82,7 +79,6 @@ mod tests {
             "Callback should be registered"
         );
 
-        // Clean up
         unsafe {
             clear_runtime_callback();
         }

bin_tests/tests/crashtracker_bin_test.rs

@@ -281,7 +281,6 @@ fn test_crash_tracking_bin_runtime_callback_frame_impl(
         p.wait().unwrap()
     });
 
-    // Runtime callback tests should crash like normal tests
     assert!(!exit_status.success());
 
     let stderr_path = format!("{0}/out.stderr", fixtures.output_dir.display());
@@ -369,8 +368,8 @@ fn validate_runtime_callback_frame_data(crash_payload: &Value) {
 
     let frames = frames.unwrap();
     assert!(
-        frames.len() >= 3,
-        "Should have at least 3 runtime frames, got {}",
+        frames.len() == 3,
+        "Should have 3 runtime frames, got {}",
         frames.len()
     );
 
@@ -384,7 +383,7 @@ fn validate_runtime_callback_frame_data(crash_payload: &Value) {
     let expected_lines = [42, 100, 10];
     let expected_columns = [15, 8, 1];
 
-    for (i, frame) in frames.iter().take(3).enumerate() {
+    for (i, frame) in frames.iter().enumerate() {
         if let Some(function) = frame.get("function") {
             assert_eq!(
                 function.as_str().unwrap(),

datadog-crashtracker-ffi/src/runtime_callback.rs

@@ -141,8 +141,6 @@ mod tests {
         let function_name = CString::new("TestModule.TestClass.test_function").unwrap();
         let file_name = CString::new("test.rb").unwrap();
 
-        // Create the internal RuntimeStackFrame directly; no conversion needed
-        // since both RuntimeStackFrame and ddog_RuntimeStackFrame have identical layouts
         let frame = RuntimeStackFrame {
             function_name: function_name.as_ptr(),
             file_name: file_name.as_ptr(),
@@ -157,34 +155,24 @@ mod tests {
     fn test_ffi_callback_registration() {
         let _guard = TEST_MUTEX.lock().unwrap();
         unsafe {
-            // Ensure clean state at start
             clear_runtime_callback();
-
-            // Test that no callback is initially registered
             assert!(!ddog_crasht_is_runtime_callback_registered());
 
-            // Test successful registration using type-safe enums
             let result = ddog_crasht_register_runtime_stack_callback(
                 test_runtime_callback,
                 CallbackType::Frame,
             );
 
             assert_eq!(result, CallbackResult::Ok);
-
-            // Verify callback is now registered
             assert!(ddog_crasht_is_runtime_callback_registered());
 
-            // Test duplicate registration fails
             let result = ddog_crasht_register_runtime_stack_callback(
                 test_runtime_callback,
                 CallbackType::Frame,
             );
             assert_eq!(result, CallbackResult::Ok);
-
-            // Callback should still be registered after successful re-registration
             assert!(ddog_crasht_is_runtime_callback_registered());
 
-            // Clean up - clear the registered callback for subsequent tests
             clear_runtime_callback();
         }
     }
@@ -195,10 +183,8 @@ mod tests {
         unsafe {
             clear_runtime_callback();
 
-            // Test that no callback is initially registered
             assert!(!ddog_crasht_is_runtime_callback_registered());
 
-            // Test registration with enum values - Python + StacktraceString
             let result = ddog_crasht_register_runtime_stack_callback(
                 test_runtime_callback,
                 CallbackType::StacktraceString,
@@ -207,15 +193,13 @@ mod tests {
             assert_eq!(result, CallbackResult::Ok);
             assert!(ddog_crasht_is_runtime_callback_registered());
 
-            // Verify callback type
             let callback_type_ptr = ddog_crasht_get_registered_callback_type();
             assert!(!callback_type_ptr.is_null());
             let callback_type_str = std::ffi::CStr::from_ptr(callback_type_ptr)
                 .to_str()
                 .unwrap();
             assert_eq!(callback_type_str, "stacktrace_string");
 
-            // Test re-registration with different values - Ruby + Frame
             let result = ddog_crasht_register_runtime_stack_callback(
                 test_runtime_callback,
                 CallbackType::Frame,

datadog-crashtracker/src/receiver/receive_report.rs

@@ -151,13 +151,20 @@ fn process_line(
                     };
                     builder.with_experimental_runtime_stack(runtime_stack)?;
                 }
+            } else {
+                builder.with_log_message(
+                    format!("Unable to parse runtime stack frames: {line}"),
+                    true,
+                )?;
             }
             StdinState::RuntimeStackFrame
         }
         StdinState::RuntimeStackString(lines)
             if line.starts_with(DD_CRASHTRACK_END_RUNTIME_STACK_STRING) =>
         {
             // Join all accumulated lines with newlines to reconstruct the full stack trace
+            // This is necessary because although the stacktrace string is sent as a single string,
+            // there may be newlines in the string
             let stacktrace_string = lines.join("\n");
             let runtime_stack = RuntimeStack {
                 format: "Datadog Runtime Callback 1.0".to_string(),

datadog-crashtracker/src/runtime_callback.rs

@@ -55,8 +55,6 @@ impl std::str::FromStr for CallbackType {
 }
 
 /// Global storage for the runtime callback
-///
-/// Uses atomic pointer to ensure safe access from signal handlers
 static RUNTIME_CALLBACK: AtomicPtr<(RuntimeStackCallback, CallbackType)> =
     AtomicPtr::new(ptr::null_mut());
 
@@ -76,7 +74,8 @@ pub struct RuntimeStackFrame {
 
 /// Function signature for runtime stack collection callbacks
 ///
-/// This callback is invoked during crash handling in a signal context, so it must be signal-safe:
+/// This callback is invoked during crash handling in a signal context, so it must be best effort
+/// signal-safe:
 /// - No dynamic memory allocation
 /// - No mutex operations
 /// - No I/O operations
@@ -148,7 +147,7 @@ pub fn register_runtime_stack_callback(
     let previous = RUNTIME_CALLBACK.swap(callback_data, Ordering::SeqCst);
 
     if !previous.is_null() {
-        // Safety: previous was returned by Box::into_raw() above or in a previous call,
+        // Safety: previous was returned by Box::into_raw() above,
         // so it's guaranteed to be a valid Box pointer. We reconstruct the Box to drop it.
         let _ = unsafe { Box::from_raw(previous) };
     }
@@ -180,8 +179,7 @@ pub unsafe fn get_registered_callback_type_enum() -> Option<CallbackType> {
 
     // Safety: callback_ptr was checked to be non-null above, and was created by
     // Box::into_raw() in register_runtime_stack_callback(), so it's a valid pointer
-    // to a properly aligned, initialized tuple. The atomic load with SeqCst ordering
-    // ensures we see the pointer after it was stored.
+    // to a properly aligned, initialized tuple.
     let (_, callback_type) = &*callback_ptr;
     Some(*callback_type)
 }
@@ -256,7 +254,9 @@ pub(crate) unsafe fn invoke_runtime_callback_with_writer<W: std::io::Write>(
     use std::sync::atomic::{AtomicPtr, Ordering};
 
     // Thread-safe storage for the current callback context
-    // Store as raw data and meta pointers
+    // Store as raw data and vtable pointers
+    // We do this because trait objects are stored as raw pointers to its definition and also
+    // the vtable for its impls, so we need to store both.
     static CURRENT_WRITER_DATA: AtomicPtr<()> = AtomicPtr::new(ptr::null_mut());
     static CURRENT_WRITER_VTABLE: AtomicPtr<()> = AtomicPtr::new(ptr::null_mut());
     static CURRENT_FRAME_COUNT: AtomicPtr<usize> = AtomicPtr::new(ptr::null_mut());