diff --git a/.azure-pipelines/main.yml b/.azure-pipelines/main.yml index 105fd89..3b6655f 100644 --- a/.azure-pipelines/main.yml +++ b/.azure-pipelines/main.yml @@ -25,6 +25,10 @@ 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' @@ -32,6 +36,9 @@ strategy: # 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) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d6cfae..a83d160 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/bin/test b/bin/test index 95a969c..f623c68 100755 --- a/bin/test +++ b/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 diff --git a/bin/test.ps1 b/bin/test.ps1 index 4266a9b..67d5329 100644 --- a/bin/test.ps1 +++ b/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 ` diff --git a/src/env.rs b/src/env.rs index e4b4eab..ccca94c 100644 --- a/src/env.rs +++ b/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 = 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>, + pub(crate) protected: Option>>, } /// 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()); } + _ => () + } } } } diff --git a/src/init.rs b/src/init.rs index 1ee8714..72103ae 100644 --- a/src/init.rs +++ b/src/init.rs @@ -68,13 +68,36 @@ pub static __PREFIX__: Lazy> = Lazy::new(|| Mutex::new(["".to pub static __MOD_IN_NAME__: Lazy = 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(env: &Env, f: F) -> os::raw::c_int where F: Fn(&Env) -> Result> + 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)) diff --git a/src/value.rs b/src/value.rs index 6756099..1a5c939 100644 --- a/src/value.rs +++ b/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 } diff --git a/test-module/tests/main.el b/test-module/tests/main.el index 6481fa3..24fb95c 100644 --- a/test-module/tests/main.el +++ b/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))))))