From b5d890a8c290b99fff3149398592ea4200a28324 Mon Sep 17 00:00:00 2001 From: Rodrigo Batista de Moraes Date: Sun, 20 Oct 2024 15:24:39 -0300 Subject: [PATCH] Fix unsoundess in libretro port --- libretro/src/lib.rs | 100 +++++++++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 42 deletions(-) diff --git a/libretro/src/lib.rs b/libretro/src/lib.rs index 1b50498..8533abe 100644 --- a/libretro/src/lib.rs +++ b/libretro/src/lib.rs @@ -1,4 +1,5 @@ use std::{ + cell::{Cell, RefCell, RefMut}, ffi::{c_char, c_void}, io::Write, mem::MaybeUninit, @@ -53,15 +54,20 @@ macro_rules! c_str { /// The Core global state. struct Core { - env_callback: retro_environment_t, - video_callback: retro_video_refresh_t, - audio_callback: retro_audio_sample_batch_t, - input_poll_callback: retro_input_poll_t, - input_state_callback: retro_input_state_t, - state: Option, + env_callback: Cell, + video_callback: Cell, + audio_callback: Cell, + input_poll_callback: Cell, + input_state_callback: Cell, + state: RefCell>, // A double buffer of the lcd screen pixels. Updated on vblank. - screen_buffer: [u8; SCREEN_WIDTH as usize * SCREEN_HEIGHT as usize], + screen_buffer: RefCell<[u8; SCREEN_WIDTH as usize * SCREEN_HEIGHT as usize]>, + + #[cfg(debug_assertions)] + /// Use to assert that the libretro API is single-threaded. (Maybe libretro can call the API + /// from multiple threads, but let's assume it doesn't) + owner_thread: std::thread::ThreadId, } impl Default for Core { fn default() -> Self { @@ -72,24 +78,32 @@ impl Default for Core { input_poll_callback: Default::default(), input_state_callback: Default::default(), state: Default::default(), - screen_buffer: [0; SCREEN_WIDTH as usize * SCREEN_HEIGHT as usize], + screen_buffer: [0; SCREEN_WIDTH as usize * SCREEN_HEIGHT as usize].into(), + + #[cfg(debug_assertions)] + owner_thread: std::thread::current().id(), } } } +impl Core { + fn state_mut(&self) -> RefMut { + RefMut::map(self.state.borrow_mut(), |x| x.as_mut().unwrap()) + } +} static mut CORE: Option = None; -fn core() -> &'static mut Core { - // RetroLib promises that it will not call the API from multiple threads, so this is safe, right? - // Actually this is unsafe because the caller could leak the mutable reference. This should be a - // RefCell or something. - // In fact, I am already doing UB, because I am calling core() in retro_run and in the - // v_blank_callback, creating two mutable references to CORE at the same time! +fn core() -> &'static Core { + // SAFETY: the libretro API is single-threaded, so we can garantee there is no concurrent acess + // to this function. CORE is never mutate after being initialized, so subsequent calls to this + // function will not invalidate previous returned references. unsafe { - if CORE.is_none() { - CORE = Some(Core::default()); - } - CORE.as_mut().unwrap() + let core = CORE.get_or_insert_with(Core::default); + + // assert this is single-threaded + debug_assert_eq!(core.owner_thread, std::thread::current().id()); + + core } } @@ -100,7 +114,7 @@ pub extern "C" fn retro_api_version() -> u32 { #[no_mangle] pub extern "C" fn retro_set_environment(callback: retro_environment_t) { - core().env_callback = callback; + core().env_callback.set(callback); let mut log_callback: retro_log_callback = retro_log_callback { log: None }; if let Some(callback) = callback { @@ -192,10 +206,10 @@ extern "C" fn retro_load_game(info: Option<&retro_game_info>) -> bool { let mut gb = GameBoy::new(None, cartridge); gb.sound.get_mut().sample_frequency = SAMPLE_RATE; gb.v_blank = Some(Box::new(|gb| { - core().screen_buffer = gb.ppu.get_mut().screen.packed(); + *core().screen_buffer.borrow_mut() = gb.ppu.get_mut().screen.packed(); })); - core().state = Some(gb); + *core().state.borrow_mut() = Some(gb); true } @@ -203,15 +217,16 @@ extern "C" fn retro_load_game(info: Option<&retro_game_info>) -> bool { #[no_mangle] pub extern "C" fn retro_run() { let core = core(); - let Some(state) = &mut core.state else { - log::error!("GameBoy is not loaded?"); - return; - }; - - state.joypad = 0xff; - if let (Some(input_poll), Some(input_state)) = - (core.input_poll_callback, core.input_state_callback) - { + // let Some(state) = &mut core.state else { + // log::error!("GameBoy is not loaded?"); + // return; + // }; + + core.state_mut().joypad = 0xff; + if let (Some(input_poll), Some(input_state)) = ( + core.input_poll_callback.get(), + core.input_state_callback.get(), + ) { let key_map = [ RETRO_DEVICE_ID_JOYPAD_RIGHT, RETRO_DEVICE_ID_JOYPAD_LEFT, @@ -229,18 +244,18 @@ pub extern "C" fn retro_run() { for (i, id) in key_map.iter().copied().enumerate() { let value = unsafe { input_state(0, RETRO_DEVICE_JOYPAD, 0, id) }; if value != 0 { - state.joypad &= !(1 << i); + core.state_mut().joypad &= !(1 << i); } } } - let target = state.clock_count + gameroy::consts::FRAME_CYCLES; - while state.clock_count < target { - Interpreter(state).interpret_op(); + let target = core.state_mut().clock_count + gameroy::consts::FRAME_CYCLES; + while core.state_mut().clock_count < target { + Interpreter(&mut core.state_mut()).interpret_op(); } - if let Some(callback) = core.video_callback { - let frame = core.screen_buffer.map(|c| { + if let Some(callback) = core.video_callback.get() { + let frame = core.screen_buffer.borrow_mut().map(|c| { /// 0RGB1555 format /// FIXME: this format is deprecated #[allow(clippy::unusual_byte_groupings)] @@ -262,9 +277,10 @@ pub extern "C" fn retro_run() { } } - if let Some(callback) = core.audio_callback { + if let Some(callback) = core.audio_callback.get() { unsafe { - let buffer = state.sound.get_mut().get_output(state.clock_count); + let clock_count = core.state_mut().clock_count; + let buffer = core.state_mut().sound.get_mut().get_output(clock_count); let buffer: Vec = buffer.into_iter().map(|x| (x as i16 - 128) * 30).collect(); (callback)(buffer.as_ptr(), buffer.len() as u64 / 2); } @@ -274,25 +290,25 @@ pub extern "C" fn retro_run() { #[no_mangle] pub extern "C" fn retro_set_video_refresh(callback: retro_video_refresh_t) { log::info!("set video refresh"); - core().video_callback = callback; + core().video_callback.set(callback); } #[no_mangle] pub extern "C" fn retro_set_audio_sample_batch(callback: retro_audio_sample_batch_t) { log::info!("set audio sample batch"); - core().audio_callback = callback; + core().audio_callback.set(callback); } #[no_mangle] pub extern "C" fn retro_set_input_poll(callback: retro_input_poll_t) { log::info!("set input poll"); - core().input_poll_callback = callback; + core().input_poll_callback.set(callback); } #[no_mangle] pub extern "C" fn retro_set_input_state(callback: retro_input_state_t) { log::info!("set input state"); - core().input_state_callback = callback; + core().input_state_callback.set(callback); } #[no_mangle]