Add rb_gc_guard! macro with same behavior as RB_GC_GUARD (#221)

* Bump `rb-sys-test-helpers` to v0.1.4

* Add `rb_gc_guard!` macro

* Ensure exact ASM is generated

* Volatile writes for protect in test helpers

* Safer version of RubyException in test helpers

* Run all tests with opt-level=3

* Don't run memory tests on windows just yet (until we have stable macros'

* Hardmode test for rb_gc_guard

* Type sig fixes

* Feature flag it

* Disabe rb_gc_guard in test helpers for now

* Bump msrv to 1.59

* Bump msrv to 1.59

* Remove ruby-lsp dep in gemfile

* Dont attempt to upload entire target dir on cargo test fail

* Remove rust toolchain file

* Avoid double panicking in test helper shutdown

* Increase timeout

* Follow the rules of MaybeUninit more closely

* Fixes

* f

* Remove unneeded feature flag

* Restrict rake-compiler version for now

.github/workflows/ci.yml

@@ -88,7 +88,7 @@ jobs:
 
       - name: 🧪 Cargo test
         shell: bash
-        run: script/ci/upload-on-failure.sh "bundle exec rake test:cargo" "cargo-test" "./target"
+        run: bundle exec rake test:cargo
 
       - name: 💎 Gem test
         run: bundle exec rake test:gem

Gemfile

@@ -7,7 +7,7 @@ gemspec path: "examples/rust_reverse"
 
 gem "rake", "~> 13.0"
 gem "minitest", "5.15.0"
-gem "rake-compiler", "~> 1.2.0"
+gem "rake-compiler", "~> 1.2.0", "< 1.2.4" # Small bug in 1.2.4 that breaks Ruby 2.5
 gem "yard"
 
 if RUBY_VERSION >= "2.6.0"

crates/rb-sys-test-helpers-macros/src/lib.rs

@@ -62,24 +62,24 @@ pub fn ruby_test(args: TokenStream, input: TokenStream) -> TokenStream {
     let vis = input.vis;
     let sig = &input.sig;
 
-    let block = quote! {
-        let ret = {
-            #block;
-        };
-        rb_sys_test_helpers::trigger_full_gc!();
-        ret
-    };
-
     let block = if gc_stress {
         quote! {
             rb_sys_test_helpers::with_gc_stress(|| {
                 #block
-            });
+            })
         }
     } else {
         quote! { #block }
     };
 
+    let block = quote! {
+        let ret = {
+            #block
+        };
+        rb_sys_test_helpers::trigger_full_gc!();
+        ret
+    };
+
     let test_fn = quote! {
         #[test]
         #(#attrs)*
@@ -89,8 +89,7 @@ pub fn ruby_test(args: TokenStream, input: TokenStream) -> TokenStream {
                     #block
                 });
 
-                match result {
-                    Ok(_) => (),
+                let ret = match result {
                     Err(err) => {
                         match std::env::var("RUST_BACKTRACE") {
                             Ok(val) if val == "1" => {
@@ -101,10 +100,13 @@ pub fn ruby_test(args: TokenStream, input: TokenStream) -> TokenStream {
                             },
                             _ => (),
                         }
-                        panic!("{}", err.inspect());
+                        Err(err)
                     },
+                    Ok(v) => Ok(v),
                 };
-            });
+
+                ret
+            }).expect("test execution failure").expect("ruby exception");
         }
     };
 

crates/rb-sys-test-helpers/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "rb-sys-test-helpers"
-version = "0.1.3"
+version = "0.1.4"
 edition = "2018"
 description = "Helpers for testing Ruby extensions from Rust"
 homepage = "https://github.com/oxidize-rb/rb-sys"

crates/rb-sys-test-helpers/src/lib.rs

@@ -7,7 +7,7 @@ mod utils;
 
 use rb_sys::{rb_errinfo, rb_intern, rb_set_errinfo, Qnil, VALUE};
 use ruby_test_executor::global_executor;
-use std::{mem::MaybeUninit, panic::UnwindSafe};
+use std::{error::Error, mem::MaybeUninit, panic::UnwindSafe};
 
 pub use rb_sys_test_helpers_macros::*;
 pub use ruby_exception::RubyException;
@@ -39,9 +39,10 @@ pub use utils::*;
 ///     assert_eq!(result, "hello world");
 /// });
 /// ```
-pub fn with_ruby_vm<F>(f: F)
+pub fn with_ruby_vm<R, F>(f: F) -> Result<R, Box<dyn Error>>
 where
-    F: FnOnce() + UnwindSafe + Send + 'static,
+    R: Send + 'static,
+    F: FnOnce() -> R + UnwindSafe + Send + 'static,
 {
     global_executor().run_test(f)
 }
@@ -64,7 +65,11 @@ where
 ///    assert_eq!(hello_world, "hello world");
 /// });
 /// ```
-pub fn with_gc_stress<T>(f: impl FnOnce() -> T + std::panic::UnwindSafe) -> T {
+pub fn with_gc_stress<R, F>(f: F) -> R
+where
+    R: Send + 'static,
+    F: FnOnce() -> R + UnwindSafe + Send + 'static,
+{
     unsafe {
         let stress_intern = rb_intern("stress\0".as_ptr() as _);
         let stress_eq_intern = rb_intern("stress=\0".as_ptr() as _);
@@ -104,32 +109,28 @@ where
     F: FnMut() -> T + std::panic::UnwindSafe,
 {
     unsafe extern "C" fn ffi_closure<T, F: FnMut() -> T>(args: VALUE) -> VALUE {
-        let args: *mut (Option<*mut F>, Option<MaybeUninit<T>>) = args as _;
-        let args = &mut *args;
-        let (func, outbuf) = args;
+        let args: *mut (Option<*mut F>, *mut Option<T>) = args as _;
+        let args = *args;
+        let (mut func, outbuf) = args;
         let func = func.take().unwrap();
         let func = &mut *func;
-        let mut outbuf = outbuf.take().unwrap();
-
         let result = func();
-        outbuf.as_mut_ptr().write(result);
-
-        outbuf.as_ptr() as _
+        outbuf.write_volatile(Some(result));
+        outbuf as _
     }
 
     unsafe {
         let mut state = 0;
         let func_ref = &Some(f) as *const _;
-        let outbuf_ref = &MaybeUninit::uninit() as *const MaybeUninit<T>;
-        let args = &(Some(func_ref), Some(outbuf_ref)) as *const _ as VALUE;
-        let outbuf_ptr = rb_sys::rb_protect(Some(ffi_closure::<T, F>), args, &mut state);
-        let outbuf_ptr: *const MaybeUninit<T> = outbuf_ptr as _;
+        let mut outbuf: MaybeUninit<Option<T>> = MaybeUninit::new(None);
+        let args = &(Some(func_ref), outbuf.as_mut_ptr() as *mut _) as *const _ as VALUE;
+        rb_sys::rb_protect(Some(ffi_closure::<T, F>), args, &mut state);
 
         if state == 0 {
-            if let Some(result) = outbuf_ptr.as_ref() {
-                Ok(result.as_ptr().read())
+            if outbuf.as_mut_ptr().read_volatile().is_some() {
+                Ok(outbuf.assume_init().expect("unreachable"))
             } else {
-                panic!("rb_protect returned a null pointer")
+                Err(RubyException::new(rb_errinfo()))
             }
         } else {
             let err = rb_errinfo();
@@ -144,12 +145,12 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_protect_returns_correct_value() {
-        with_ruby_vm(|| {
-            let result = protect(|| "my val");
+    fn test_protect_returns_correct_value() -> Result<(), Box<dyn Error>> {
+        let ret = with_ruby_vm(|| protect(|| "my val"))?;
+
+        assert_eq!(ret, Ok("my val"));
 
-            assert_eq!(result, Ok("my val"));
-        });
+        Ok(())
     }
 
     #[test]
@@ -160,6 +161,7 @@ mod tests {
             });
 
             assert!(result.is_err());
-        });
+        })
+        .unwrap();
     }
 }

crates/rb-sys-test-helpers/src/ruby_exception.rs

@@ -8,7 +8,7 @@ use std::ffi::CStr;
 
 /// A simple wrapper around a Ruby exception that provides some convenience
 /// methods for testing.
-#[derive(Clone, Copy, Eq, PartialEq)]
+#[derive(Clone, Eq, PartialEq)]
 pub struct RubyException {
     value: VALUE,
 }
@@ -20,15 +20,15 @@ impl RubyException {
     }
 
     /// Get the message of the Ruby exception.
-    pub fn message(self) -> Option<String> {
+    pub fn message(&self) -> Option<String> {
         unsafe {
             rb_funcall_typed!(self.value, "message", [], RUBY_T_STRING)
                 .map(|mut message| rstring_to_string!(message))
         }
     }
 
     /// Get the full message of the Ruby exception.
-    pub fn full_message(self) -> Option<String> {
+    pub fn full_message(&self) -> Option<String> {
         unsafe {
             if let Some(mut message) =
                 rb_funcall_typed!(self.value, "full_message", [], RUBY_T_STRING)
@@ -42,7 +42,7 @@ impl RubyException {
     }
 
     /// Get the backtrace string of the Ruby exception.
-    pub fn backtrace(self) -> Option<String> {
+    pub fn backtrace(&self) -> Option<String> {
         unsafe {
             if let Some(backtrace) = rb_funcall_typed!(self.value, "backtrace", [], RUBY_T_ARRAY) {
                 let mut backtrace = rb_ary_join(backtrace, rb_str_new("\n".as_ptr() as _, 1));
@@ -60,7 +60,7 @@ impl RubyException {
     }
 
     /// Get the inspect string of the Ruby exception.
-    pub fn inspect(self) -> String {
+    pub fn inspect(&self) -> String {
         unsafe {
             if let Some(mut inspect) = rb_funcall_typed!(self.value, "inspect", [], RUBY_T_STRING) {
                 rstring_to_string!(inspect)
@@ -71,14 +71,20 @@ impl RubyException {
     }
 
     /// Get the class name of the Ruby exception.
-    pub fn classname(self) -> String {
+    pub fn classname(&self) -> String {
         unsafe {
             let classname = rb_class2name(rb_obj_class(self.value));
             CStr::from_ptr(classname).to_string_lossy().into_owned()
         }
     }
 }
 
+// impl Drop for RubyException {
+//     fn drop(&mut self) {
+//         rb_sys::rb_gc_guard!(self.value);
+//     }
+// }
+
 impl std::fmt::Debug for RubyException {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let message = self.message();
@@ -113,7 +119,7 @@ mod tests {
     use rb_sys::rb_eval_string;
 
     #[test]
-    fn test_exception() {
+    fn test_exception() -> Result<(), Box<dyn std::error::Error>> {
         with_ruby_vm(|| {
             let exception = protect(|| unsafe {
                 rb_eval_string("raise 'oh no'\0".as_ptr() as _);