Merge pull request #31 from ubolonton/call-unprotected

Allow selectively enabling no-GC-protection optimization

Author: ubolonton

CHANGELOG.md

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## [Unreleased]
 - Reworked `Vector`, making it iterable.
 - Changed `Env::vector` to return a `Value`.
+- Added `call_unprotected` variants to `Value::call`, `Env::call`, `GlobalRef::call`, to enable certain optimizations.
 
 ## [0.13.0] - 2020-03-11
 - Added `GlobalRef`, which allows keeping Lisp values around without an `Env`.

src/call.rs

@@ -33,7 +33,25 @@ impl<'e> Value<'e> {
     /// ```
     ///
     /// [`IntoLisp`]: trait.IntoLisp.html
+    #[inline]
     pub fn call<A>(self, args: A) -> Result<Value<'e>> where A: IntoLispArgs<'e> {
+        // Safety: The returned value is explicitly protected.
+        unsafe { self.call_unprotected(args).map(|v| v.protect()) }
+    }
+
+    /// Like [`call`], except that the returned `Value` is not protected against
+    /// Emacs GC's [bug #31238], which caused [issue #2].
+    ///
+    /// # Safety
+    ///
+    /// This can be used as an optimization, in situations when the returned `Value` is unused,
+    /// or when its usage is shorter than the lifespan of the underlying Lisp object.
+    ///
+    /// [`call`]: #method.call
+    /// [bug #31238]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238
+    /// [issue #2]: https://github.com/ubolonton/emacs-module-rs/issues/2
+    #[allow(unused_unsafe)]
+    pub unsafe fn call_unprotected<A>(self, args: A) -> Result<Value<'e>> where A: IntoLispArgs<'e> {
         let env = self.env;
         let mut lisp_args = args.into_lisp_args(env)?;
         let lisp_args: &mut [emacs_value] = lisp_args.borrow_mut();
@@ -42,7 +60,7 @@ impl<'e> Value<'e> {
         // Safety:
         // - ptr comes from a locally-owned value.
         // - length is ensured to be valid by IntoLispArgs implementation.
-        unsafe_raw_call_value!(env, funcall, self.raw, length, ptr)
+        unsafe_raw_call_value_unprotected!(env, funcall, self.raw, length, ptr)
     }
 }
 
@@ -78,12 +96,33 @@ impl Env {
     /// [`IntoLisp`]: trait.IntoLisp.html
     #[inline]
     pub fn call<'e, F, A>(&'e self, func: F, args: A) -> Result<Value<'_>>
-    where
-        F: IntoLispCallable<'e>,
-        A: IntoLispArgs<'e>,
+        where
+            F: IntoLispCallable<'e>,
+            A: IntoLispArgs<'e>,
     {
         func.into_lisp_callable(self)?.call(args)
     }
+
+    /// Like [`call`], except that the returned [`Value`] is not protected against
+    /// Emacs GC's [bug #31238], which caused [issue #2].
+    ///
+    /// # Safety
+    ///
+    /// This can be used as an optimization, in situations when the returned [`Value`] is unused,
+    /// or when its usage is shorter than the lifespan of the underlying Lisp object.
+    ///
+    /// [`call`]: #method.call
+    /// [`Value`]: struct.Value.html
+    /// [bug #31238]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238
+    /// [issue #2]: https://github.com/ubolonton/emacs-module-rs/issues/2
+    #[inline]
+    pub unsafe fn call_unprotected<'e, F, A>(&'e self, func: F, args: A) -> Result<Value<'_>>
+        where
+            F: IntoLispCallable<'e>,
+            A: IntoLispArgs<'e>,
+    {
+        func.into_lisp_callable(self)?.call_unprotected(args)
+    }
 }
 
 impl GlobalRef {
@@ -101,6 +140,26 @@ impl GlobalRef {
     {
         self.bind(env).call(args)
     }
+
+    /// Like [`call`], except that the returned [`Value`] is not protected against
+    /// Emacs GC's [bug #31238], which caused [issue #2].
+    ///
+    /// # Safety
+    ///
+    /// This can be used as an optimization, in situations when the returned [`Value`] is unused,
+    /// or when its usage is shorter than the lifespan of the underlying Lisp object.
+    ///
+    /// [`call`]: #method.call
+    /// [`Value`]: struct.Value.html
+    /// [bug #31238]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238
+    /// [issue #2]: https://github.com/ubolonton/emacs-module-rs/issues/2
+    #[inline]
+    pub unsafe fn call_unprotected<'e, A>(&'e self, env: &'e Env, args: A) -> Result<Value<'_>>
+        where
+            A: IntoLispArgs<'e>,
+    {
+        self.bind(env).call_unprotected(args)
+    }
 }
 
 // We can implement IntoLispArgs for IntoLisp types (after breaking up this implementation).

src/error.rs

@@ -95,7 +95,7 @@ impl TempValue {
     ///
     /// [`Env`]: struct.Env.html
     pub unsafe fn value<'e>(&self, env: &'e Env) -> Value<'e> {
-        Value::new_protected(self.raw, env)
+        Value::new(self.raw, env).protect()
     }
 }
 

src/global.rs

@@ -63,6 +63,7 @@ impl GlobalRef {
     /// [`Value`]: struct.Value.html
     #[inline]
     pub fn bind<'e, 'g: 'e>(&'g self, env: &'e Env) -> Value<'e> {
+        // Safety: This global ref keeps the underlying Lisp object alive.
         unsafe { Value::new(self.raw, env) }
     }
 

src/macros.rs

@@ -32,13 +32,33 @@ macro_rules! unsafe_raw_call {
 }
 
 /// Calls a raw function that returns an emacs_value, then handles any pending non-local exit.
+/// Returns a [`Value`].
+///
+/// [`Value`]: struct.Value.html
 macro_rules! unsafe_raw_call_value {
+    ($env:expr, $name:ident $(, $args:expr)*) => {
+        unsafe_raw_call_value_unprotected!($env, $name $(, $args)*).map(|v| v.protect())
+    };
+}
+
+/// Like [`unsafe_raw_call_value!`], except that the returned [`Value`] is not protected against
+/// Emacs GC's [bug #31238], which caused [issue #2].
+///
+/// # Safety
+///
+/// This can be used as an optimization, in situations when the returned [`Value`] is unused,
+/// or when its usage is shorter than the lifespan of the underlying Lisp object.
+///
+/// [`unsafe_raw_call_value!`]: macro.unsafe_raw_call_value.html
+/// [`Value`]: struct.Value.html
+/// [bug #31238]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238
+/// [issue #2]: https://github.com/ubolonton/emacs-module-rs/issues/2
+macro_rules! unsafe_raw_call_value_unprotected {
     ($env:expr, $name:ident $(, $args:expr)*) => {
         {
             let result: $crate::Result<$crate::raw::emacs_value> = unsafe_raw_call!($env, $name $(, $args)*);
             result.map(|raw| unsafe {
-                // TODO: In some cases, we can get away without protection. Try optimizing them.
-                $crate::Value::new_protected(raw, $env)
+                $crate::Value::new(raw, $env)
             })
         }
     };

src/types/float.rs

@@ -8,6 +8,6 @@ impl FromLisp<'_> for f64 {
 
 impl IntoLisp<'_> for f64 {
     fn into_lisp(self, env: &Env) -> Result<Value<'_>> {
-        unsafe_raw_call_value!(env, make_float, self)
+        unsafe_raw_call_value_unprotected!(env, make_float, self)
     }
 }

src/types/integer.rs

@@ -39,7 +39,7 @@ int_from_lisp!(usize);
 
 impl IntoLisp<'_> for i64 {
     fn into_lisp(self, env: &Env) -> Result<Value<'_>> {
-        unsafe_raw_call_value!(env, make_integer, self)
+        unsafe_raw_call_value_unprotected!(env, make_integer, self)
     }
 }
 

src/types/vector.rs

@@ -41,8 +41,11 @@ impl<'e> Vector<'e> {
     pub fn get<T: FromLisp<'e>>(&self, i: usize) -> Result<T> {
         let v = self.value;
         let env = v.env;
-        // Safety: Same lifetime. Emacs does bound checking.
-        unsafe_raw_call_value!(env, vec_get, v.raw, i as isize)?.into_rust()
+        // Safety:
+        // - Same lifetime.
+        // - Emacs does bound checking.
+        // - Value doesn't need protection because we are done with it while the vector still lives.
+        unsafe_raw_call_value_unprotected!(env, vec_get, v.raw, i as isize)?.into_rust()
     }
 
     pub fn set<T: IntoLisp<'e>>(&self, i: usize, value: T) -> Result<()> {

src/value.rs

@@ -23,29 +23,31 @@ impl<'e> Value<'e> {
     ///
     /// # Safety
     ///
-    /// The raw value must not live longer than the given [`Env`].
+    /// The raw value must be valid for as long as the returned `Value` is used. This usually means
+    /// "as long as [`Env`] is alive", but can be shorter, depending on how the returned `Value` is
+    /// used at call site.
     ///
     /// [`Env`]: struct.Env.html
     #[doc(hidden)]
     pub unsafe fn new(raw: emacs_value, env: &'e Env) -> Self {
         Self { raw, env }
     }
 
-    /// Constructs a new `Value` and "roots" its underlying raw value (GC-managed) during the
-    /// lifetime of the given [`Env`]. Module code should not call this directly. It is public only
-    /// for some internal macros to use.
+    /// Protects this value by registering with its [`Env`], effectively "rooting" the underlying
+    /// Lisp object during the lifetime of the [`Env`].
     ///
-    /// # Safety
-    ///
-    /// The raw value must still be alive. This function is needed to protect new values returned
-    /// from Emacs runtime, due to [this issue](https://github.com/ubolonton/emacs-module-rs/issues/2).
+    /// Module code should not call this directly. It is public only for certain internal macros and
+    /// functions to work around Emacs GC's [bug #31238], which caused [issue #2].
     ///
     /// [`Env`]: struct.Env.html
-    #[allow(unused_unsafe)]
+    /// [bug #31238]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238
+    /// [issue #2]: https://github.com/ubolonton/emacs-module-rs/issues/2
     #[doc(hidden)]
-    pub unsafe fn new_protected(raw: emacs_value, env: &'e Env) -> Self {
+    #[inline]
+    pub fn protect(self) -> Self {
+        let Self { env, raw } = self;
         env.protected.borrow_mut().push(unsafe_raw_call_no_exit!(env, make_global_ref, raw));
-        Self::new(raw, env)
+        self
     }
 
     pub fn is_not_nil(&self) -> bool {

test-module/src/test_lifetime.rs

@@ -57,6 +57,22 @@ fn gc_after_new_string(env: &Env) -> Result<Value<'_>> {
     }, print)
 }
 
+// Primitive types supposedly have no issue.
+#[defun(mod_in_name = false)]
+fn gc_after_new_int(env: &Env) -> Result<Value<'_>> {
+    create_collect_use(env, 2, || {
+        5.into_lisp(env)
+    }, print)
+}
+
+// Primitive types supposedly have no issue.
+#[defun(mod_in_name = false)]
+fn gc_after_new_float(env: &Env) -> Result<Value<'_>> {
+    create_collect_use(env, 2, || {
+        5.8.into_lisp(env)
+    }, print)
+}
+
 // Before fixing:
 // - macOS: Segmentation fault
 // - Linux: Segmentation fault