From 6f71e9935f03fd7a86cbbeefc486533d0e2e0318 Mon Sep 17 00:00:00 2001 From: DavisVaughan Date: Fri, 9 Jun 2023 17:24:40 -0400 Subject: [PATCH] Add lock checking intermediate layer --- crates/ark/src/interface.rs | 10 +++++++--- crates/ark/src/kernel.rs | 24 +++++++++++++----------- crates/ark/src/lsp/backend.rs | 16 ++++++++-------- crates/ark/tests/environment.rs | 6 ++++-- crates/harp/harp-macros/src/lib.rs | 30 ++++++++++++++++++++++++++++++ crates/harp/src/lib.rs | 1 + crates/harp/src/protect.rs | 15 ++++++++------- crates/harp/src/r_api.rs | 30 ++++++++++++++++++++++++++++++ crates/harp/src/utils.rs | 22 +++++++++++++--------- crates/harp/src/vector/mod.rs | 3 ++- 10 files changed, 116 insertions(+), 41 deletions(-) create mode 100644 crates/harp/src/r_api.rs diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 18201a933..cc137fd90 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -26,6 +26,7 @@ use crossbeam::channel::Sender; use harp::interrupts::RInterruptsSuspendedScope; use harp::lock::R_RUNTIME_LOCK; use harp::lock::R_RUNTIME_LOCK_COUNT; +use harp::r_lock; use harp::routines::r_register_routines; use harp::utils::r_get_option; use libR_sys::*; @@ -135,7 +136,7 @@ extern "C" fn handle_interrupt(_signal: libc::c_int) { /// The global R kernel state. pub static mut KERNEL: Option>> = None; -/// A lock guard, used to manage access to the R runtime. The main thread holds +/// A lock guard, used to manage access to the R runtime. The main thread holds /// the lock by default, but releases it at opportune times to allow the LSP to /// access the R runtime where appropriate. pub static mut R_RUNTIME_LOCK_GUARD: Option> = None; @@ -264,6 +265,7 @@ pub extern "C" fn r_read_console( }, Err(error) => { + // Take back the lock after we've timed out. unsafe { R_RUNTIME_LOCK_GUARD = Some(R_RUNTIME_LOCK.lock()) }; use RecvTimeoutError::*; @@ -482,8 +484,11 @@ fn complete_execute_request(req: &Request, prompt_recv: &Receiver) { // Signal prompt EVENTS.console_prompt.emit(()); + // Get necessary prompts + let (continue_prompt, default_prompt) = + r_lock! { (r_get_option::("continue"), r_get_option::("prompt")) }; + // The request is incomplete if we see the continue prompt - let continue_prompt = unsafe { r_get_option::("continue") }; let continue_prompt = unwrap!(continue_prompt, Err(error) => { warn!("Failed to read R continue prompt: {}", error); return kernel.finish_request(); @@ -495,7 +500,6 @@ fn complete_execute_request(req: &Request, prompt_recv: &Receiver) { } // Figure out what the default prompt looks like. - let default_prompt = unsafe { r_get_option::("prompt") }; let default_prompt = unwrap!(default_prompt, Err(error) => { warn!("Failed to read R prompt: {}", error); return kernel.finish_request(); diff --git a/crates/ark/src/kernel.rs b/crates/ark/src/kernel.rs index be762e9e6..fa3f8e281 100644 --- a/crates/ark/src/kernel.rs +++ b/crates/ark/src/kernel.rs @@ -229,17 +229,19 @@ impl Kernel { let mut data = serde_json::Map::new(); data.insert("text/plain".to_string(), json!("")); - // Include HTML representation of data.frame - // TODO: Do we need to hold the R lock here? - let value = unsafe { Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")) }; - if r_is_data_frame(value) { - match Kernel::to_html(value) { - Ok(html) => data.insert("text/html".to_string(), json!(html)), - Err(error) => { - error!("{:?}", error); - None - }, - }; + r_lock! { + let value = Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")); + + // Include HTML representation of data.frame + if r_is_data_frame(value) { + match Kernel::to_html(value) { + Ok(html) => data.insert("text/html".to_string(), json!(html)), + Err(error) => { + error!("{:?}", error); + None + }, + }; + } } if let Err(err) = self diff --git a/crates/ark/src/lsp/backend.rs b/crates/ark/src/lsp/backend.rs index 5e867228f..940af1c07 100644 --- a/crates/ark/src/lsp/backend.rs +++ b/crates/ark/src/lsp/backend.rs @@ -436,12 +436,10 @@ impl LanguageServer for Backend { return Ok(item); }); - unsafe { - unwrap!(resolve_completion_item(&mut item, &data), Err(error) => { - error!("{:?}", error); - return Ok(item); - }); - } + unwrap!(r_lock! { resolve_completion_item(&mut item, &data) }, Err(error) => { + error!("{:?}", error); + return Ok(item); + }); Ok(item) } @@ -464,7 +462,9 @@ impl LanguageServer for Backend { }); // request hover information - let result = unsafe { hover(&document, &context) }; + let result = r_lock! { + hover(&document, &context) + }; // unwrap errors let result = unwrap!(result, Err(error) => { @@ -494,7 +494,7 @@ impl LanguageServer for Backend { // request signature help let position = params.text_document_position_params.position; - let result = unsafe { signature_help(document.value(), &position) }; + let result = r_lock! { signature_help(document.value(), &position) }; // unwrap errors let result = unwrap!(result, Err(error) => { diff --git a/crates/ark/tests/environment.rs b/crates/ark/tests/environment.rs index 3f9119b3a..9d03df1af 100644 --- a/crates/ark/tests/environment.rs +++ b/crates/ark/tests/environment.rs @@ -59,10 +59,12 @@ fn test_environment_list() { // Create a new environment handler and give it a view of the test // environment we created. - let test_env_view = RObject::view(test_env.sexp); let incoming_tx = comm.incoming_tx.clone(); let outgoing_rx = comm.outgoing_rx.clone(); - REnvironment::start(test_env_view, comm); + r_lock! { + let test_env_view = RObject::view(test_env.sexp); + REnvironment::start(test_env_view, comm.clone()) + } // Ensure we get a list of variables after initialization let msg = outgoing_rx.recv().unwrap(); diff --git a/crates/harp/harp-macros/src/lib.rs b/crates/harp/harp-macros/src/lib.rs index 4be93c9e6..7abb97718 100644 --- a/crates/harp/harp-macros/src/lib.rs +++ b/crates/harp/harp-macros/src/lib.rs @@ -149,6 +149,36 @@ pub fn vector(_attr: TokenStream, item: TokenStream) -> TokenStream { all.into() } +#[proc_macro_attribute] +pub fn lock(_attr: TokenStream, item: TokenStream) -> TokenStream { + // Get the pieces of the function being wrapped + let syn::ItemFn { + attrs, + vis, + sig, + block, + } = syn::parse(item).unwrap(); + + let stmts = &block.stmts; + + // Generate error message + let message = format!("Can't use `{}()` without holding the R lock.", sig.ident); + + let out = quote! { + #(#attrs)* #vis #sig { + + #[cfg(debug_assertions)] + unsafe { + std::assert!(crate::lock::R_RUNTIME_LOCK.is_locked(), #message); + } + + #(#stmts)* + } + }; + + out.into() +} + #[proc_macro_attribute] pub fn register(_attr: TokenStream, item: TokenStream) -> TokenStream { // Get metadata about the function being registered. diff --git a/crates/harp/src/lib.rs b/crates/harp/src/lib.rs index ca2b36ba1..8c0f58adb 100644 --- a/crates/harp/src/lib.rs +++ b/crates/harp/src/lib.rs @@ -13,6 +13,7 @@ pub mod interrupts; pub mod lock; pub mod object; pub mod protect; +pub mod r_api; pub mod routines; pub mod string; pub mod symbol; diff --git a/crates/harp/src/protect.rs b/crates/harp/src/protect.rs index 271359db5..088cdb2f6 100644 --- a/crates/harp/src/protect.rs +++ b/crates/harp/src/protect.rs @@ -5,7 +5,9 @@ // // -use libR_sys::*; +use crate::r_api::Rf_protect; +use crate::r_api::Rf_unprotect; +use crate::r_api::SEXP; // NOTE: The RProtect struct uses R's stack-based object protection, and so is // only appropriate for R objects with 'automatic' lifetime. In general, this @@ -16,21 +18,20 @@ pub struct RProtect { } impl RProtect { - /// SAFETY: Assumes that the R lock is held. - pub unsafe fn new() -> Self { + pub fn new() -> Self { Self { count: 0 } } - /// SAFETY: Assumes that the R lock is held. - pub unsafe fn add(&mut self, object: SEXP) -> SEXP { + /// SAFETY: Requires that the R lock is held. + pub fn add(&mut self, object: SEXP) -> SEXP { self.count += 1; return Rf_protect(object); } } impl Drop for RProtect { - /// SAFETY: Assumes that the R lock is held. + /// SAFETY: Requires that the R lock is held. fn drop(&mut self) { - unsafe { Rf_unprotect(self.count) } + Rf_unprotect(self.count) } } diff --git a/crates/harp/src/r_api.rs b/crates/harp/src/r_api.rs new file mode 100644 index 000000000..c052d9ed7 --- /dev/null +++ b/crates/harp/src/r_api.rs @@ -0,0 +1,30 @@ +// +// r_api.rs +// +// Copyright (C) 2023 Posit Software, PBC. All rights reserved. +// +// + +#![allow(non_snake_case)] + +pub use libR_sys::SEXP; + +#[harp_macros::lock] +pub fn Rf_xlength(x: SEXP) -> isize { + unsafe { libR_sys::Rf_xlength(x) } +} + +#[harp_macros::lock] +pub fn TYPEOF(x: SEXP) -> std::os::raw::c_int { + unsafe { libR_sys::TYPEOF(x) } +} + +#[harp_macros::lock] +pub fn Rf_protect(x: SEXP) -> SEXP { + unsafe { libR_sys::Rf_protect(x) } +} + +#[harp_macros::lock] +pub fn Rf_unprotect(count: i32) { + unsafe { libR_sys::Rf_unprotect(count) } +} diff --git a/crates/harp/src/utils.rs b/crates/harp/src/utils.rs index 63ca859a5..6166cd445 100644 --- a/crates/harp/src/utils.rs +++ b/crates/harp/src/utils.rs @@ -23,6 +23,9 @@ use crate::exec::RFunction; use crate::exec::RFunctionExt; use crate::object::RObject; use crate::protect::RProtect; +use crate::r_api::Rf_xlength; +use crate::r_api::SEXP; +use crate::r_api::TYPEOF; use crate::r_symbol; use crate::symbol::RSymbol; use crate::vector::CharacterVector; @@ -87,6 +90,14 @@ impl Sxpinfo { } } +pub fn r_length(x: SEXP) -> isize { + Rf_xlength(x) +} + +pub fn r_typeof(x: SEXP) -> u32 { + TYPEOF(x) as u32 +} + pub fn r_assert_type(object: SEXP, expected: &[u32]) -> Result { let actual = r_typeof(object); @@ -107,7 +118,7 @@ pub unsafe fn r_assert_capacity(object: SEXP, required: usize) -> Result } pub fn r_assert_length(object: SEXP, expected: usize) -> Result { - let actual = unsafe { Rf_xlength(object) as usize }; + let actual = r_length(object) as usize; if actual != expected { return Err(Error::UnexpectedLength(actual, expected)); } @@ -207,7 +218,7 @@ pub fn r_vec_shape(value: SEXP) -> String { let dim = RObject::new(Rf_getAttrib(value, R_DimSymbol)); if r_is_null(*dim) { - format!("{}", Rf_xlength(value)) + format!("{}", r_length(value)) } else { let dim = IntegerVector::new_unchecked(*dim); dim.iter().map(|d| d.unwrap()).join(", ") @@ -224,13 +235,6 @@ pub fn r_altrep_class(object: SEXP) -> String { format!("{}::{}", pkg, klass) } -pub fn r_typeof(object: SEXP) -> u32 { - // SAFETY: The type of an R object is typically considered constant, - // and TYPEOF merely queries the R type directly from the SEXPREC struct. - let object = object.into(); - unsafe { TYPEOF(object) as u32 } -} - pub unsafe fn r_type2char>(kind: T) -> String { let kind = Rf_type2char(kind.into()); let cstr = CStr::from_ptr(kind); diff --git a/crates/harp/src/vector/mod.rs b/crates/harp/src/vector/mod.rs index 17697b201..b74cab4af 100644 --- a/crates/harp/src/vector/mod.rs +++ b/crates/harp/src/vector/mod.rs @@ -10,6 +10,7 @@ use libR_sys::*; use crate::error::Result; use crate::utils::r_assert_capacity; use crate::utils::r_assert_type; +use crate::utils::r_length; pub mod character_vector; pub use character_vector::CharacterVector; @@ -87,7 +88,7 @@ pub trait Vector { ::Item: AsRef; unsafe fn len(&self) -> usize { - Rf_xlength(self.data()) as usize + r_length(self.data()) as usize } fn format_one(&self, x: Self::Type) -> String;