From 5b8412656c16e3577d87d1bcb2d3f190c520d4f5 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Wed, 14 Jun 2023 01:09:58 -0400 Subject: [PATCH 1/2] Initialize jars in-place Requires unsafe, since Rust currently doesn't have built-in support for guaranteed in-place initialization. Unfortunately, this unsafety propagates to making `Jar` unsafe to implement in order to guarantee that the jar is initialized, but since the preferred way to implement it is via `salsa::jar`, this won't impact most users. --- components/salsa-2022-macros/src/db.rs | 25 ++- components/salsa-2022-macros/src/jar.rs | 12 +- components/salsa-2022/src/jar.rs | 14 +- components/salsa-2022/src/plumbing.rs | 27 +++ components/salsa-2022/src/storage.rs | 4 +- .../tests/create-empty-database.rs | 35 ++++ .../tests/create-large-jar-database.rs | 154 ++++++++++++++++++ 7 files changed, 255 insertions(+), 16 deletions(-) create mode 100644 salsa-2022-tests/tests/create-empty-database.rs create mode 100644 salsa-2022-tests/tests/create-large-jar-database.rs diff --git a/components/salsa-2022-macros/src/db.rs b/components/salsa-2022-macros/src/db.rs index 6c77ef62..6fa2a989 100644 --- a/components/salsa-2022-macros/src/db.rs +++ b/components/salsa-2022-macros/src/db.rs @@ -1,5 +1,5 @@ use proc_macro2::Literal; -use syn::Token; +use syn::{spanned::Spanned, Token}; // Source: // @@ -84,6 +84,12 @@ fn as_salsa_database_impl(input: &syn::ItemStruct) -> syn::ItemImpl { fn has_jars_impl(args: &Args, input: &syn::ItemStruct, storage: &syn::Ident) -> syn::ItemImpl { let jar_paths: Vec<&syn::Path> = args.jar_paths.iter().collect(); + let jar_field_names: Vec<_> = args + .jar_paths + .iter() + .zip(0..) + .map(|(p, i)| syn::LitInt::new(&format!("{}", i), p.span())) + .collect(); let db = &input.ident; parse_quote! { // ANCHOR: HasJars @@ -100,12 +106,17 @@ fn has_jars_impl(args: &Args, input: &syn::ItemStruct, storage: &syn::Ident) -> } // ANCHOR: create_jars - fn create_jars(routes: &mut salsa::routes::Routes) -> Self::Jars { - ( - #( - <#jar_paths as salsa::jar::Jar>::create_jar(routes), - )* - ) + fn create_jars(routes: &mut salsa::routes::Routes) -> Box { + unsafe { + salsa::plumbing::create_jars_inplace::<#db>(|jars| { + #( + unsafe { + let place = std::ptr::addr_of_mut!((*jars).#jar_field_names); + <#jar_paths as salsa::jar::Jar>::init_jar(place, routes); + } + )* + }) + } } // ANCHOR_END: create_jars } diff --git a/components/salsa-2022-macros/src/jar.rs b/components/salsa-2022-macros/src/jar.rs index 311b9259..a0b2358e 100644 --- a/components/salsa-2022-macros/src/jar.rs +++ b/components/salsa-2022-macros/src/jar.rs @@ -106,21 +106,23 @@ pub(crate) fn jar_impl( .fields .iter() .zip(0..) - .map(|(f, i)| Ident::new(&format!("i{}", i), f.ty.span())) + .map(|(f, i)| syn::LitInt::new(&format!("{}", i), f.ty.span())) .collect(); // ANCHOR: create_jar quote! { - impl<'salsa_db> salsa::jar::Jar<'salsa_db> for #jar_struct { + unsafe impl<'salsa_db> salsa::jar::Jar<'salsa_db> for #jar_struct { type DynDb = dyn #jar_trait + 'salsa_db; - fn create_jar(routes: &mut salsa::routes::Routes) -> Self + unsafe fn init_jar(place: *mut Self, routes: &mut salsa::routes::Routes) where DB: salsa::storage::JarFromJars + salsa::storage::DbWithJar, { #( - let #field_var_names = <#field_tys as salsa::storage::IngredientsFor>::create_ingredients(routes); + unsafe { + std::ptr::addr_of_mut!((*place).#field_var_names) + .write(<#field_tys as salsa::storage::IngredientsFor>::create_ingredients(routes)); + } )* - Self(#(#field_var_names),*) } } } diff --git a/components/salsa-2022/src/jar.rs b/components/salsa-2022/src/jar.rs index 36c41eb0..35c4438e 100644 --- a/components/salsa-2022/src/jar.rs +++ b/components/salsa-2022/src/jar.rs @@ -5,10 +5,20 @@ use crate::{ use super::routes::Routes; -pub trait Jar<'db>: Sized { +/// Representative trait of a salsa jar +/// +/// # Safety +/// +/// `init_jar` must fully initialize the jar +pub unsafe trait Jar<'db>: Sized { type DynDb: ?Sized + HasJar + Database + 'db; - fn create_jar(routes: &mut Routes) -> Self + /// Initializes the jar at `place` + /// + /// # Safety + /// + /// `place` must be a valid pointer to this jar + unsafe fn init_jar(place: *mut Self, routes: &mut Routes) where DB: JarFromJars + DbWithJar; } diff --git a/components/salsa-2022/src/plumbing.rs b/components/salsa-2022/src/plumbing.rs index 8b137891..65a06451 100644 --- a/components/salsa-2022/src/plumbing.rs +++ b/components/salsa-2022/src/plumbing.rs @@ -1 +1,28 @@ +use std::{alloc, ptr}; +use crate::storage::HasJars; + +/// Initializes the `DB`'s jars in-place +/// +/// # Safety: +/// +/// `init` must fully initialize all of jars fields +pub unsafe fn create_jars_inplace(init: impl FnOnce(*mut DB::Jars)) -> Box { + let layout = alloc::Layout::new::(); + + if layout.size() == 0 { + // SAFETY: This is the recommended way of creating a Box + // to a ZST in the std docs + unsafe { Box::from_raw(ptr::NonNull::dangling().as_ptr()) } + } else { + // SAFETY: We've checked that the size isn't 0 + let place = unsafe { alloc::alloc_zeroed(layout) }; + let place = place.cast::(); + + init(place); + + // SAFETY: Caller invariant requires that `init` must've + // initialized all of the fields + unsafe { Box::from_raw(place) } + } +} diff --git a/components/salsa-2022/src/storage.rs b/components/salsa-2022/src/storage.rs index 13c2e853..49b0a50a 100644 --- a/components/salsa-2022/src/storage.rs +++ b/components/salsa-2022/src/storage.rs @@ -60,7 +60,7 @@ where let jars = DB::create_jars(&mut routes); Self { shared: Shared { - jars: Some(Arc::new(jars)), + jars: Some(Arc::from(jars)), cvar: Arc::new(Default::default()), }, routes: Arc::new(routes), @@ -192,7 +192,7 @@ pub trait HasJars: HasJarsDyn + Sized { /// and it will also cancel any ongoing work in the current revision. fn jars_mut(&mut self) -> (&mut Self::Jars, &mut Runtime); - fn create_jars(routes: &mut Routes) -> Self::Jars; + fn create_jars(routes: &mut Routes) -> Box; } pub trait DbWithJar: HasJar + Database { diff --git a/salsa-2022-tests/tests/create-empty-database.rs b/salsa-2022-tests/tests/create-empty-database.rs new file mode 100644 index 00000000..9064a244 --- /dev/null +++ b/salsa-2022-tests/tests/create-empty-database.rs @@ -0,0 +1,35 @@ +//! Tests that we can create a database of only 0-size jars without invoking UB + +use salsa::storage::HasJars; + +#[salsa::jar(db = Db)] +struct Jar(); + +trait Db: salsa::DbWithJar {} + +#[salsa::db(Jar)] +#[derive(Default)] +struct Database { + storage: salsa::Storage, +} + +impl salsa::Database for Database {} + +impl Db for Database {} + +#[test] +fn execute() { + let db = Database::default(); + let jars = db.storage.jars().0; + + ensure_init(jars); +} + +fn ensure_init(place: *const ::Jars) { + use std::mem::forget; + use std::ptr::addr_of; + + // SAFETY: Intentionally tries to access potentially uninitialized memory, + // so that miri can catch if we accidentally forget to initialize the memory. + forget(unsafe { addr_of!((*place).0).read() }); +} diff --git a/salsa-2022-tests/tests/create-large-jar-database.rs b/salsa-2022-tests/tests/create-large-jar-database.rs new file mode 100644 index 00000000..8f753d88 --- /dev/null +++ b/salsa-2022-tests/tests/create-large-jar-database.rs @@ -0,0 +1,154 @@ +//! Tests that we can create a database with very large jars without invoking UB + +use salsa::storage::HasJars; + +#[salsa::db(jar1::Jar1, jar2::Jar2, jar3::Jar3, jar4::Jar4)] +#[derive(Default)] +struct Database { + storage: salsa::Storage, +} + +impl salsa::Database for Database {} + +#[test] +fn execute() { + let db = Database::default(); + let jars = db.storage.jars().0; + + ensure_init(jars); +} + +fn ensure_init(place: *const ::Jars) { + use std::mem::forget; + use std::ptr::addr_of; + + // SAFETY: Intentionally tries to access potentially uninitialized memory, + // so that miri can catch if we accidentally forget to initialize the memory. + forget(unsafe { addr_of!((*place).0).read() }); + forget(unsafe { addr_of!((*place).1).read() }); + forget(unsafe { addr_of!((*place).2).read() }); + forget(unsafe { addr_of!((*place).3).read() }); +} + +macro_rules! make_jarX { + ($jarX:ident, $JarX:ident) => { + mod $jarX { + #[salsa::jar(db = Db)] + pub(crate) struct $JarX(T1); + + pub(crate) trait Db: salsa::DbWithJar<$JarX> {} + + impl Db for DB where DB: salsa::DbWithJar<$JarX> {} + + #[salsa::tracked(jar = $JarX)] + struct T1 { + a0: String, + a1: String, + a2: String, + a3: String, + a4: String, + a5: String, + a6: String, + a7: String, + a8: String, + a9: String, + a10: String, + a11: String, + a12: String, + a13: String, + a14: String, + a15: String, + a16: String, + a17: String, + a18: String, + a19: String, + a20: String, + a21: String, + a22: String, + a23: String, + a24: String, + a25: String, + a26: String, + a27: String, + a28: String, + a29: String, + a30: String, + a31: String, + a32: String, + a33: String, + a34: String, + a35: String, + a36: String, + a37: String, + a38: String, + a39: String, + a40: String, + a41: String, + a42: String, + a43: String, + a44: String, + a45: String, + a46: String, + a47: String, + a48: String, + a49: String, + a50: String, + a51: String, + a52: String, + a53: String, + a54: String, + a55: String, + a56: String, + a57: String, + a58: String, + a59: String, + a60: String, + a61: String, + a62: String, + a63: String, + a64: String, + a65: String, + a66: String, + a67: String, + a68: String, + a69: String, + a70: String, + a71: String, + a72: String, + a73: String, + a74: String, + a75: String, + a76: String, + a77: String, + a78: String, + a79: String, + a80: String, + a81: String, + a82: String, + a83: String, + a84: String, + a85: String, + a86: String, + a87: String, + a88: String, + a89: String, + a90: String, + a91: String, + a92: String, + a93: String, + a94: String, + a95: String, + a96: String, + a97: String, + a98: String, + a99: String, + a100: String, + } + } + }; +} + +make_jarX!(jar1, Jar1); +make_jarX!(jar2, Jar2); +make_jarX!(jar3, Jar3); +make_jarX!(jar4, Jar4); From f1a141a6c036c0a09d66999b29b6396b8e1bd847 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Sun, 18 Jun 2023 00:13:54 -0400 Subject: [PATCH 2/2] Update "Jars and Ingredients" to reflect the in-place init changes --- book/src/plumbing/jars_and_ingredients.md | 6 +++--- components/salsa-2022-macros/src/jar.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/book/src/plumbing/jars_and_ingredients.md b/book/src/plumbing/jars_and_ingredients.md index 506a460f..2928bac3 100644 --- a/book/src/plumbing/jars_and_ingredients.md +++ b/book/src/plumbing/jars_and_ingredients.md @@ -195,16 +195,16 @@ The `Default` implementation for `Storage` does the work: First, it creates an empty `Routes` instance. Then it invokes the `DB::create_jars` method. -The implementation of this method is defined by the `#[salsa::db]` macro; it simply invokes the `Jar::create_jar` method on each of the jars: +The implementation of this method is defined by the `#[salsa::db]` macro; it invokes `salsa::plumbing::create_jars_inplace` to allocate memory for the jars, and then invokes the `Jar::init_jar` method on each of the jars to initialize them: ```rust,ignore {{#include ../../../components/salsa-2022-macros/src/db.rs:create_jars}} ``` -This implementation for `create_jar` is geneated by the `#[salsa::jar]` macro, and simply walks over the representative type for each salsa item and asks *it* to create its ingredients +This implementation for `init_jar` is generated by the `#[salsa::jar]` macro, and simply walks over the representative type for each salsa item and asks *it* to create its ingredients ```rust,ignore -{{#include ../../../components/salsa-2022-macros/src/jar.rs:create_jar}} +{{#include ../../../components/salsa-2022-macros/src/jar.rs:init_jar}} ``` The code to create the ingredients for any particular item is generated by their associated macros (e.g., `#[salsa::tracked]`, `#[salsa::input]`), but it always follows a particular structure. diff --git a/components/salsa-2022-macros/src/jar.rs b/components/salsa-2022-macros/src/jar.rs index a0b2358e..44b9fbdd 100644 --- a/components/salsa-2022-macros/src/jar.rs +++ b/components/salsa-2022-macros/src/jar.rs @@ -108,7 +108,7 @@ pub(crate) fn jar_impl( .zip(0..) .map(|(f, i)| syn::LitInt::new(&format!("{}", i), f.ty.span())) .collect(); - // ANCHOR: create_jar + // ANCHOR: init_jar quote! { unsafe impl<'salsa_db> salsa::jar::Jar<'salsa_db> for #jar_struct { type DynDb = dyn #jar_trait + 'salsa_db; @@ -126,7 +126,7 @@ pub(crate) fn jar_impl( } } } - // ANCHOR_END: create_jar + // ANCHOR_END: init_jar } pub(crate) fn jar_struct(input: &ItemStruct) -> ItemStruct {