From dfedd7b2c4a7edc64a1866ee41e79b5445bdd49a Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 3 Feb 2022 15:58:00 +0100 Subject: [PATCH] Enable assert_no_alloc in debug builds --- Cargo.lock | 8 ++ Cargo.toml | 9 ++ plugins/gain/Cargo.toml | 2 +- plugins/sine/Cargo.toml | 2 +- src/wrapper/util.rs | 17 ++++ src/wrapper/vst3.rs | 215 +++++++++++++++++++++------------------- 6 files changed, 147 insertions(+), 106 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ee1effda..542a8fb1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8,6 +8,12 @@ version = "1.0.53" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94a45b455c14666b85fc40a019e8ab9eb75e3a124e05494f5397122bc9eb06e0" +[[package]] +name = "assert_no_alloc" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55ca83137a482d61d916ceb1eba52a684f98004f18e0cafea230fe5579c178a3" + [[package]] name = "autocfg" version = "1.0.1" @@ -142,6 +148,8 @@ dependencies = [ name = "nih_plug" version = "0.1.0" dependencies = [ + "assert_no_alloc", + "cfg-if", "crossbeam", "lazy_static", "nih_plug_derive", diff --git a/Cargo.toml b/Cargo.toml index 728bcbd5..51dfeeec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,9 +18,18 @@ members = [ nih_plug_derive = { path = "nih_plug_derive" } crossbeam = "0.8" +cfg-if = "1.0" lazy_static = "1.4" parking_lot = "0.12" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" vst3-sys = { git = "https://github.com/robbert-vdh/vst3-sys.git", branch = "fix/atomic-reference-count" } widestring = "1.0.0-beta.1" + +assert_no_alloc = { version = "1.1", optional = true } + +[features] +default = [] +# Enabling this feature will cause the plugin to terminate when allocations +# occur in the processing function while compiling in debug mode. +assert_process_allocs = ["assert_no_alloc"] diff --git a/plugins/gain/Cargo.toml b/plugins/gain/Cargo.toml index 7b6e3125..44f36b18 100644 --- a/plugins/gain/Cargo.toml +++ b/plugins/gain/Cargo.toml @@ -8,6 +8,6 @@ license = "GPL-3.0-or-later" crate-type = ["cdylib"] [dependencies] -nih_plug = { path = "../../" } +nih_plug = { path = "../../", features = ["assert_process_allocs"] } parking_lot = "0.12" diff --git a/plugins/sine/Cargo.toml b/plugins/sine/Cargo.toml index ed9a3f5a..5e90bd12 100644 --- a/plugins/sine/Cargo.toml +++ b/plugins/sine/Cargo.toml @@ -8,4 +8,4 @@ license = "GPL-3.0-or-later" crate-type = ["cdylib"] [dependencies] -nih_plug = { path = "../../" } +nih_plug = { path = "../../", features = ["assert_process_allocs"] } diff --git a/src/wrapper/util.rs b/src/wrapper/util.rs index bc059156..3c6cee8a 100644 --- a/src/wrapper/util.rs +++ b/src/wrapper/util.rs @@ -19,6 +19,10 @@ use std::os::raw::c_char; use vst3_sys::vst::TChar; use widestring::U16CString; +#[cfg(all(debug_assertions, feature = "assert_process_allocs"))] +#[global_allocator] +static A: assert_no_alloc::AllocDisabler = assert_no_alloc::AllocDisabler; + /// A Rabin fingerprint based string hash for parameter ID strings. pub fn hash_param_id(id: &str) -> u32 { let mut overflow; @@ -83,3 +87,16 @@ pub fn u16strlcpy(dest: &mut [TChar], src: &str) { dest[..copy_len].copy_from_slice(&src_utf16_chars_signed[..copy_len]); dest[copy_len] = 0; } + +/// A wrapper around the entire process function, including the plugin wrapper parts. This sets up +/// `assert_no_alloc` if needed, while also making sure that things like FTZ are set up correctly if +/// the host has not already done so. +pub fn process_wrapper T>(f: F) -> T { + cfg_if::cfg_if! { + if #[cfg(all(debug_assertions, feature = "assert_process_allocs"))] { + assert_no_alloc::assert_no_alloc(f) + } else { + f() + } + } +} diff --git a/src/wrapper/vst3.rs b/src/wrapper/vst3.rs index a3b5cbc3..747f2013 100644 --- a/src/wrapper/vst3.rs +++ b/src/wrapper/vst3.rs @@ -47,7 +47,7 @@ use crate::param::range::Range; use crate::param::Param; use crate::plugin::{BufferConfig, BusConfig, Plugin, ProcessStatus, Vst3Plugin}; use crate::wrapper::state::{ParamValue, State}; -use crate::wrapper::util::{hash_param_id, strlcpy, u16strlcpy}; +use crate::wrapper::util::{hash_param_id, process_wrapper, strlcpy, u16strlcpy}; // Alias needed for the VST3 attribute macro use vst3_sys as vst3_com; @@ -972,124 +972,131 @@ impl IAudioProcessor for Wrapper<'_, P> { unsafe fn process(&self, data: *mut vst3_sys::vst::ProcessData) -> tresult { check_null_ptr!(data); - // We need to handle incoming automation first - let data = &*data; - let sample_rate = self - .inner - .current_buffer_config - .load() - .map(|c| c.sample_rate); - if let Some(param_changes) = data.input_param_changes.upgrade() { - let num_param_queues = param_changes.get_parameter_count(); - for change_queue_idx in 0..num_param_queues { - if let Some(param_change_queue) = - param_changes.get_parameter_data(change_queue_idx).upgrade() - { - let param_hash = param_change_queue.get_parameter_id(); - let num_changes = param_change_queue.get_point_count(); - - // TODO: Handle sample accurate parameter changes, possibly in a similar way to - // the smoothing - let mut sample_offset = 0i32; - let mut value = 0.0f64; - if num_changes > 0 - && param_change_queue.get_point( - num_changes - 1, - &mut sample_offset, - &mut value, - ) == kResultOk + // Panic on allocations if the `assert_process_allocs` feature has been enabled, and make + // sure that FTZ is set up correctly + process_wrapper(|| { + // We need to handle incoming automation first + let data = &*data; + let sample_rate = self + .inner + .current_buffer_config + .load() + .map(|c| c.sample_rate); + if let Some(param_changes) = data.input_param_changes.upgrade() { + let num_param_queues = param_changes.get_parameter_count(); + for change_queue_idx in 0..num_param_queues { + if let Some(param_change_queue) = + param_changes.get_parameter_data(change_queue_idx).upgrade() { - self.inner - .set_normalized_value_by_hash(param_hash, value, sample_rate); + let param_hash = param_change_queue.get_parameter_id(); + let num_changes = param_change_queue.get_point_count(); + + // TODO: Handle sample accurate parameter changes, possibly in a similar way to + // the smoothing + let mut sample_offset = 0i32; + let mut value = 0.0f64; + if num_changes > 0 + && param_change_queue.get_point( + num_changes - 1, + &mut sample_offset, + &mut value, + ) == kResultOk + { + self.inner + .set_normalized_value_by_hash(param_hash, value, sample_rate); + } } } } - } - // It's possible the host only wanted to send new parameter values - if data.num_outputs == 0 { - nih_log!("VST3 parameter flush"); - return kResultOk; - } - - // The setups we suppport are: - // - 1 input bus - // - 1 output bus - // - 1 input bus and 1 output bus - nih_debug_assert!( - data.num_inputs >= 0 - && data.num_inputs <= 1 - && data.num_outputs >= 0 - && data.num_outputs <= 1, - "The host provides more than one input or output bus" - ); - nih_debug_assert_eq!( - data.symbolic_sample_size, - vst3_sys::vst::SymbolicSampleSizes::kSample32 as i32 - ); - nih_debug_assert!(data.num_samples >= 0); - - let num_output_channels = (*data.outputs).num_channels as usize; - check_null_ptr_msg!( - "Process output pointer is null", - data.outputs, - (*data.outputs).buffers, - ); - - // This vector has been reallocated to contain enough slices as there are output channels - let mut output_buffer = self.inner.output_buffer.write(); - { - let output_slices = output_buffer.as_raw_vec(); - nih_debug_assert_eq!(num_output_channels, output_slices.len()); - for (output_channel_idx, output_channel_slice) in output_slices.iter_mut().enumerate() { - // SAFETY: These pointers may not be valid outside of this function even though - // their lifetime is equal to this structs. This is still safe because they are only - // dereferenced here later as part of this process function. - *output_channel_slice = std::slice::from_raw_parts_mut( - *((*data.outputs).buffers as *mut *mut f32).add(output_channel_idx), - data.num_samples as usize, - ); + // It's possible the host only wanted to send new parameter values + if data.num_outputs == 0 { + nih_log!("VST3 parameter flush"); + return kResultOk; } - } - // Most hosts process data in place, in which case we don't need to do any copying - // ourselves. If the pointers do not alias, then we'll do the copy here and then the plugin - // can just do normal in place processing. - if !data.inputs.is_null() { - let num_input_channels = (*data.inputs).num_channels as usize; + // The setups we suppport are: + // - 1 input bus + // - 1 output bus + // - 1 input bus and 1 output bus nih_debug_assert!( - num_input_channels <= num_output_channels, - "Stereo to mono and similar configurations are not supported" + data.num_inputs >= 0 + && data.num_inputs <= 1 + && data.num_outputs >= 0 + && data.num_outputs <= 1, + "The host provides more than one input or output bus" ); - for input_channel_idx in 0..cmp::min(num_input_channels, num_output_channels) { - let output_channel_ptr = - *((*data.outputs).buffers as *mut *mut f32).add(input_channel_idx); - let input_channel_ptr = - *((*data.inputs).buffers as *const *const f32).add(input_channel_idx); - if input_channel_ptr != output_channel_ptr { - ptr::copy_nonoverlapping( - input_channel_ptr, - output_channel_ptr, + nih_debug_assert_eq!( + data.symbolic_sample_size, + vst3_sys::vst::SymbolicSampleSizes::kSample32 as i32 + ); + nih_debug_assert!(data.num_samples >= 0); + + let num_output_channels = (*data.outputs).num_channels as usize; + check_null_ptr_msg!( + "Process output pointer is null", + data.outputs, + (*data.outputs).buffers, + ); + + // This vector has been reallocated to contain enough slices as there are output + // channels + let mut output_buffer = self.inner.output_buffer.write(); + { + let output_slices = output_buffer.as_raw_vec(); + nih_debug_assert_eq!(num_output_channels, output_slices.len()); + for (output_channel_idx, output_channel_slice) in + output_slices.iter_mut().enumerate() + { + // SAFETY: These pointers may not be valid outside of this function even though + // their lifetime is equal to this structs. This is still safe because they are + // only dereferenced here later as part of this process function. + *output_channel_slice = std::slice::from_raw_parts_mut( + *((*data.outputs).buffers as *mut *mut f32).add(output_channel_idx), data.num_samples as usize, ); } } - } - match self - .inner - .plugin - .write() - // SAFETY: Same here - .process(&mut output_buffer, self.inner.as_ref()) - { - ProcessStatus::Error(err) => { - nih_debug_assert_failure!("Process error: {}", err); - - kResultFalse + // Most hosts process data in place, in which case we don't need to do any copying + // ourselves. If the pointers do not alias, then we'll do the copy here and then the + // plugin can just do normal in place processing. + if !data.inputs.is_null() { + let num_input_channels = (*data.inputs).num_channels as usize; + nih_debug_assert!( + num_input_channels <= num_output_channels, + "Stereo to mono and similar configurations are not supported" + ); + for input_channel_idx in 0..cmp::min(num_input_channels, num_output_channels) { + let output_channel_ptr = + *((*data.outputs).buffers as *mut *mut f32).add(input_channel_idx); + let input_channel_ptr = + *((*data.inputs).buffers as *const *const f32).add(input_channel_idx); + if input_channel_ptr != output_channel_ptr { + ptr::copy_nonoverlapping( + input_channel_ptr, + output_channel_ptr, + data.num_samples as usize, + ); + } + } } - _ => kResultOk, - } + + match self + .inner + .plugin + .write() + // SAFETY: Same here + .process(&mut output_buffer, self.inner.as_ref()) + { + ProcessStatus::Error(err) => { + nih_debug_assert_failure!("Process error: {}", err); + + kResultFalse + } + _ => kResultOk, + } + }) } unsafe fn get_tail_samples(&self) -> u32 {