Merge pull request #35 from ubolonton/conditionalize-gc-bug-workaround

Disable the workaround for Emacs GC bug if it's fixed (i.e. Emacs 27+)

.azure-pipelines/main.yml

@@ -25,13 +25,20 @@ strategy:
     macos-10.14-emacs-26.3:
       IMAGE_NAME: 'macos-10.14'
       EVM_EMACS: emacs-26.3
+    macos-10.14-emacs-27.1:
+      IMAGE_NAME: 'macos-10.14'
+      EVM_EMACS: emacs-27.1
+
     # Emacs is getting "Killed 9" on these agents, probably due to macOS 10.15 updates.
     # macos-10.15-emacs-25.3:
     #   IMAGE_NAME: 'macos-10.15'
     #   EVM_EMACS: emacs-25.3
     # macos-10.15-emacs-26.3:
     #   IMAGE_NAME: 'macos-10.15'
     #   EVM_EMACS: emacs-26.3
+    macos-10.15-emacs-27.1:
+      IMAGE_NAME: 'macos-10.15'
+      EVM_EMACS: emacs-27.1
 
     ubuntu-16.04-emacs-25.3:
       IMAGE_NAME: 'ubuntu-16.04'
@@ -40,12 +47,19 @@ strategy:
     # ubuntu-16.04-emacs-26.3:
     #   IMAGE_NAME: 'ubuntu-16.04'
     #   EVM_EMACS: emacs-26.3
+    ubuntu-16.04-emacs-27.1:
+      IMAGE_NAME: 'ubuntu-16.04'
+      EVM_EMACS: emacs-27.1
+
     ubuntu-18.04-emacs-25.3:
       IMAGE_NAME: 'ubuntu-18.04'
       EVM_EMACS: emacs-25.3
     ubuntu-18.04-emacs-26.3:
       IMAGE_NAME: 'ubuntu-18.04'
       EVM_EMACS: emacs-26.3
+    ubuntu-18.04-emacs-27.1:
+      IMAGE_NAME: 'ubuntu-18.04'
+      EVM_EMACS: emacs-27.1
 
 pool:
   vmImage: $(IMAGE_NAME)

CHANGELOG.md

@@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## [Unreleased]
+- Disabled the [workaround](https://github.com/ubolonton/emacs-module-rs/pull/3) for Emacs's [GC bug #31238](https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238) if possible (i.e. in Emacs 27+, where the bug was fixed). This should speed up module code in newer Emacs versions.
 
 ## [0.14.1] - 2020-10-14
 - Fixed proc macro's hygiene issues, for compatibility with Rust 1.47. Example: `emacs-tree-sitter` [fails to compile](https://github.com/ubolonton/emacs-tree-sitter/issues/62).
@@ -19,7 +20,7 @@ For details, see Rust's [release note](https://github.com/rust-lang/rust/blob/1.
 - Reduced indirection when calling common built-in subroutines through `Env`.
 - Removed `module_init!`, `export_functions!`,  and their aliases.
 - Replaced `lazy_static` dependency with `once_cell`.
-- Fixed memory leaks caused by the memory safety [fix](https://github.com/ubolonton/emacs-module-rs/pull/3) for [#2](https://github.com/ubolonton/emacs-module-rs/issues/2).
+- Fixed memory leaks caused by the [workaround](https://github.com/ubolonton/emacs-module-rs/pull/3) for Emacs's [GC bug #31238](https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238), which caused [issue #2](https://github.com/ubolonton/emacs-module-rs/issues/2).
 
 ## [0.12.3] - 2020-02-18
 - Added `Value::car`, `Value::cdr`.

bin/test

@@ -16,7 +16,7 @@ else
     $EMACS --version
     echo "Testing $MODULE_FULL"
 
-    $EMACS -batch --directory "$MODULE_DIR" \
+    EMACS_MODULE_RS_DEBUG=1 $EMACS -batch --directory "$MODULE_DIR" \
            -l ert \
            -l "$PROJECT_ROOT/test-module/tests/main.el" \
            -f ert-run-tests-batch-and-exit

bin/test.ps1

@@ -23,6 +23,7 @@ if ($args[0] -eq "watch") {
     $ErrorActionPreference = 'Continue'
     $env:PROJECT_ROOT = $project_root
     $env:MODULE_DIR = $module_dir
+    $env:EMACS_MODULE_RS_DEBUG = 1
     emacs --version
     emacs --batch --directory "$module_dir" `
       -l ert `

src/env.rs

@@ -1,23 +1,42 @@
-use std::{cell::RefCell, ffi::CString};
+use std::{
+    cell::RefCell,
+    ffi::CString,
+    mem::MaybeUninit,
+};
 
-use emacs_module::{emacs_runtime, emacs_env, emacs_value};
+use once_cell::sync::OnceCell;
+
+use emacs_module::{emacs_env, emacs_runtime, emacs_value};
 
 use crate::{subr, error, Value, Result, IntoLisp, call::IntoLispArgs, GlobalRef};
-use std::mem::MaybeUninit;
+
+/// Whether the Emacs process that loaded this module has fixed [bug #31238], which caused
+/// [issue #2]. If it has, the initialization logic will disable the [workaround] of protecting
+/// every newly created [`Value`].
+///
+/// [bug #31238]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31238
+/// [issue #2]: https://github.com/ubolonton/emacs-module-rs/issues/2
+/// [workaround]: https://github.com/ubolonton/emacs-module-rs/pull/3
+/// [`Value`]: struct.Value.html
+pub static HAS_FIXED_GC_BUG_31238: OnceCell<bool> = OnceCell::new();
 
 /// Main point of interaction with the Lisp runtime.
 #[derive(Debug)]
 pub struct Env {
     pub(crate) raw: *mut emacs_env,
     /// Raw values "rooted" during the lifetime of this `Env`.
-    pub(crate) protected: RefCell<Vec<emacs_value>>,
+    pub(crate) protected: Option<RefCell<Vec<emacs_value>>>,
 }
 
 /// Public APIs.
 impl Env {
     #[doc(hidden)]
     pub unsafe fn new(raw: *mut emacs_env) -> Self {
-        let protected = RefCell::new(vec![]);
+        let protected = if *HAS_FIXED_GC_BUG_31238.get().unwrap_or(&false) {
+            None
+        } else {
+            Some(RefCell::new(vec![]))
+        };
         Self { raw, protected }
     }
 
@@ -36,8 +55,10 @@ impl Env {
     // For testing.
     #[doc(hidden)]
     pub unsafe fn free_last_protected(&self) -> Result<()>{
-        let gr = GlobalRef::from_raw(*self.protected.borrow().last().unwrap());
-        gr.free(self)?;
+        if let Some(protected) = &self.protected {
+            let gr = GlobalRef::from_raw(*protected.borrow().last().unwrap());
+            gr.free(self)?;
+        }
         Ok(())
     }
 
@@ -84,27 +105,29 @@ impl Env {
 // TODO: Add tests to make sure the protected values are not leaked.
 impl Drop for Env {
     fn drop(&mut self) {
-        #[cfg(build = "debug")]
-        println!("Unrooting {} values protected by {:?}", self.protected.borrow().len(), self);
-        // If the `defun` returned a non-local exit, we clear it so that `free_global_ref` doesn't
-        // bail out early. Afterwards we restore the non-local exit status and associated data.
-        // It's kind of like an `unwind-protect`.
-        let mut symbol = MaybeUninit::uninit();
-        let mut data = MaybeUninit::uninit();
-        // TODO: Check whether calling non_local_exit_check first makes a difference in performance.
-        let status = self.non_local_exit_get(&mut symbol, &mut data);
-        if status == error::SIGNAL || status == error::THROW {
-            self.non_local_exit_clear();
-        }
-        for raw in self.protected.borrow().iter() {
-            // TODO: Do we want to stop if `free_global_ref` returned a non-local exit?
-            // Safety: We assume user code doesn't directly call C function `free_global_ref`.
-            unsafe_raw_call_no_exit!(self, free_global_ref, *raw);
-        }
-        match status {
-            error::SIGNAL => unsafe { self.signal(symbol.assume_init(), data.assume_init()); }
-            error::THROW => unsafe { self.throw(symbol.assume_init(), data.assume_init()); }
-            _ => ()
+        if let Some(protected) = &self.protected {
+            #[cfg(build = "debug")]
+            println!("Unrooting {} values protected by {:?}", protected.borrow().len(), self);
+            // If the `defun` returned a non-local exit, we clear it so that `free_global_ref` doesn't
+            // bail out early. Afterwards we restore the non-local exit status and associated data.
+            // It's kind of like an `unwind-protect`.
+            let mut symbol = MaybeUninit::uninit();
+            let mut data = MaybeUninit::uninit();
+            // TODO: Check whether calling non_local_exit_check first makes a difference in performance.
+            let status = self.non_local_exit_get(&mut symbol, &mut data);
+            if status == error::SIGNAL || status == error::THROW {
+                self.non_local_exit_clear();
+            }
+            for raw in protected.borrow().iter() {
+                // TODO: Do we want to stop if `free_global_ref` returned a non-local exit?
+                // Safety: We assume user code doesn't directly call C function `free_global_ref`.
+                unsafe_raw_call_no_exit!(self, free_global_ref, *raw);
+            }
+            match status {
+                error::SIGNAL => unsafe { self.signal(symbol.assume_init(), data.assume_init()); }
+                error::THROW => unsafe { self.throw(symbol.assume_init(), data.assume_init()); }
+                _ => ()
+            }
         }
     }
 }

src/init.rs

@@ -68,13 +68,36 @@ pub static __PREFIX__: Lazy<Mutex<[String; 2]>> = Lazy::new(|| Mutex::new(["".to
 
 pub static __MOD_IN_NAME__: Lazy<AtomicBool> = Lazy::new(|| AtomicBool::new(true));
 
+fn debugging() -> bool {
+    std::env::var("EMACS_MODULE_RS_DEBUG").unwrap_or_default() == "1"
+}
+
+fn check_gc_bug_31238(env: &Env) -> Result<()> {
+    let version = env.call("default-value", [env.intern("emacs-version")?])?;
+    let fixed = env.call("version<=", ("27", version))?.is_not_nil();
+    if debugging() {
+        env.call("set", (
+            env.intern("module-rs-disable-gc-bug-31238-workaround")?,
+            // Can't use true/false directly because symbol mod's globals have not been initialized.
+            env.intern(match fixed {
+                true => "t",
+                false => "nil",
+            })?
+        ))?;
+    }
+    crate::env::HAS_FIXED_GC_BUG_31238.get_or_init(|| fixed);
+    Ok(())
+}
+
 #[inline]
 pub fn initialize<F>(env: &Env, f: F) -> os::raw::c_int
 where
     F: Fn(&Env) -> Result<Value<'_>> + panic::RefUnwindSafe,
 {
     let env = panic::AssertUnwindSafe(env);
-    let result = panic::catch_unwind(|| match env.define_errors().and_then(|_| f(&env)) {
+    let result = panic::catch_unwind(|| match env.define_errors()
+        .and_then(|_| check_gc_bug_31238(&env))
+        .and_then(|_| f(&env)) {
         Ok(_) => 0,
         Err(e) => {
             env.message(format!("Error during initialization: {:#?}", e))

src/value.rs

@@ -46,7 +46,9 @@ impl<'e> Value<'e> {
     #[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));
+        if let Some(protected) = &env.protected {
+            protected.borrow_mut().push(unsafe_raw_call_no_exit!(env, make_global_ref, raw));
+        }
         self
     }
 

test-module/tests/main.el

@@ -319,6 +319,8 @@
     (ert-skip "--module-assertions is not supported"))
   (when (eq system-type 'windows-nt)
     (ert-skip "We don't know how to correctly handle failed PowerShell subprocess"))
+  (when (bound-and-true-p module-rs-disable-gc-bug-31238-workaround)
+    (ert-skip "Workaround for the GC bug 31238 was already disabled"))
   (should (string-match-p
            "Emacs value not found in"
            (cadr (t/get-error (t/run-in-sub-process 't/free-global-ref-after-normal-return))))))
@@ -328,6 +330,8 @@
     (ert-skip "--module-assertions is not supported"))
   (when (eq system-type 'windows-nt)
     (ert-skip "We don't know how to correctly handle failed PowerShell subprocess"))
+  (when (bound-and-true-p module-rs-disable-gc-bug-31238-workaround)
+    (ert-skip "Workaround for the GC bug 31238 was already disabled"))
   (should (string-match-p
            "Emacs value not found in"
            (cadr (t/get-error (t/run-in-sub-process 't/free-global-ref-after-error))))))