-
Notifications
You must be signed in to change notification settings - Fork 90
Make unprefixed consistently override the system allocator #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,8 @@ | ||
[workspace] | ||
members = ["jemallocator", "jemallocator-global", "jemalloc-ctl", "jemalloc-sys"] | ||
members = [ | ||
"jemallocator", | ||
"jemallocator-global", | ||
"jemalloc-ctl", | ||
"jemalloc-sys", | ||
"test-dylib", | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -890,3 +890,86 @@ pub type extent_merge_t = unsafe extern "C" fn( | |
mod env; | ||
|
||
pub use env::*; | ||
|
||
// When using the `"override_allocator_on_supported_platforms"` feature flag, | ||
// the user wants us to globally override the system allocator. | ||
// | ||
// However, since we build `jemalloc` as a static library (an archive), the | ||
// linker may decide to not care about our overrides if it can't directly see | ||
// references to the symbols, see the following link for details: | ||
// <https://maskray.me/blog/2021-06-20-symbol-processing#archive-processing> | ||
// | ||
// This is problematic if `jemalloc_sys` is used from a library that by itself | ||
// doesn't allocate, while invoking other shared libraries that do. | ||
// | ||
// Another especially problematic case would be something like the following: | ||
// | ||
// ``` | ||
// // Call `malloc` whose symbol is looked up statically. | ||
// let ptr = libc::malloc(42); | ||
// | ||
// // But use a dynamically looked up `free`. | ||
// let free = libc::dlsym(null_mut(), c"free".as_ptr()); | ||
// let free = transmute::<*mut c_void, unsafe extern "C" fn(*mut c_void)>(free); | ||
// free(ptr); | ||
// ``` | ||
// | ||
// Since if the `malloc` and `free` provided by `jemalloc` end up in different | ||
// object files in the archive (NOTE: In practice, this is unlikely to be an | ||
// issue, since `jemalloc.c` contains all the implementations and is compiled | ||
// as a single object file), the linker would think that only `malloc` was | ||
// used, and would never load the `free` that we also want (and hence we'd end | ||
// up executing jemalloc's `malloc` and the system's `free`, which is UB). | ||
// | ||
// To avoid this problem, we make sure that all the allocator functions are | ||
// visible to the linker, such that it will always override all of them. | ||
// | ||
// We do this by referencing these symbols in `#[used]` statics, which makes | ||
// them known to `rustc`, which will reference them in a `symbols.o` stub file | ||
// that is later passed to the linker. See the following link for details on | ||
// how this works: | ||
// <https://github.com/rust-lang/rust/pull/95604> | ||
|
||
#[cfg(all( | ||
feature = "override_allocator_on_supported_platforms", | ||
not(target_vendor = "apple") | ||
))] | ||
mod set_up_statics { | ||
use super::*; | ||
|
||
#[used] | ||
static USED_MALLOC: unsafe extern "C" fn(usize) -> *mut c_void = malloc; | ||
#[used] | ||
static USED_CALLOC: unsafe extern "C" fn(usize, usize) -> *mut c_void = calloc; | ||
#[used] | ||
static USED_POSIX_MEMALIGN: unsafe extern "C" fn(*mut *mut c_void, usize, usize) -> c_int = | ||
posix_memalign; | ||
#[used] | ||
static USED_ALIGNED_ALLOC: unsafe extern "C" fn(usize, usize) -> *mut c_void = aligned_alloc; | ||
#[used] | ||
static USED_REALLOC: unsafe extern "C" fn(*mut c_void, usize) -> *mut c_void = realloc; | ||
#[used] | ||
static USED_FREE: unsafe extern "C" fn(*mut c_void) = free; | ||
} | ||
|
||
// On macOS, jemalloc doesn't directly override malloc/free, but instead | ||
// registers itself with the allocator's zone APIs in a ctor (`zone_register` | ||
// is marked with `__attribute__((constructor))`). | ||
// | ||
// Similarly to above though, for the Mach-O linker to actually consider ctors | ||
// as "used" when defined in an archive member in a static library, so we need | ||
// to explicitly reference the function via. Rust's `#[used]`. | ||
|
||
#[cfg(all( | ||
feature = "override_allocator_on_supported_platforms", | ||
target_vendor = "apple" | ||
))] | ||
#[used] | ||
static USED_ZONE_REGISTER: unsafe extern "C" fn() = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better make it a new feature so that we can land it without a new minor version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd argue this is more in the category "bugfix" rather than "feature"; the Or do you fear this will have a chance of breaking something for users? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It actually works if
Yes, it's known not to override system allocator on MacOS. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. In the last commit, I've changed it to:
I've also made the statics only get emitted with that feature enabled. |
||
extern "C" { | ||
#[cfg_attr(prefixed, link_name = "_rjem_je_zone_register")] | ||
#[cfg_attr(not(prefixed), link_name = "je_zone_register")] | ||
fn zone_register(); | ||
} | ||
zone_register | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
[package] | ||
name = "test-dylib" | ||
version = "0.0.0" | ||
license = "MIT OR Apache-2.0" | ||
description = "A test helper for jemalloc-sys" | ||
edition = "2018" | ||
publish = false | ||
|
||
[dependencies] | ||
libc = { version = "^0.2.8", default-features = false } | ||
tikv-jemalloc-sys = { path = "../jemalloc-sys" } | ||
|
||
[build-dependencies] | ||
cc = "^1.0.13" | ||
|
||
[features] | ||
override_allocator_on_supported_platforms = [ | ||
"tikv-jemalloc-sys/override_allocator_on_supported_platforms", | ||
] | ||
|
||
[[bin]] | ||
name = "test-dylib" | ||
test = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
//! Build shared library `dep.c`. | ||
use std::{env, path::PathBuf}; | ||
|
||
fn main() { | ||
println!("cargo:rerun-if-changed=src/dep.c"); | ||
|
||
let out_dir = PathBuf::from(std::env::var_os("OUT_DIR").unwrap()); | ||
|
||
// NOTE: Only for testing, extension is wrong when cross-compiling. | ||
let dylib = out_dir.join(format!( | ||
"{}dep{}", | ||
env::consts::DLL_PREFIX, | ||
env::consts::DLL_SUFFIX | ||
)); | ||
|
||
let status = cc::Build::new() | ||
.get_compiler() | ||
.to_command() | ||
.arg("src/dep.c") | ||
.arg("-shared") | ||
.arg("-o") | ||
.arg(&dylib) | ||
.status() | ||
.unwrap(); | ||
assert!(status.success()); | ||
|
||
println!("cargo:rustc-link-lib=dylib=dep"); | ||
println!("cargo:rustc-link-search=native={}", out_dir.display()); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
#define _GNU_SOURCE | ||
#include <stdlib.h> | ||
#include <stdio.h> | ||
#include <dlfcn.h> | ||
|
||
const char* dep_lookup_malloc_address(void) { | ||
Dl_info info; | ||
if (!dladdr((void *)malloc, &info)) { | ||
printf("failed finding `malloc`\n"); | ||
abort(); | ||
} | ||
return info.dli_fname; | ||
} | ||
|
||
void* dep_malloc(size_t size) { | ||
return malloc(size); | ||
} | ||
|
||
void dep_free(void* ptr) { | ||
free(ptr); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any references on how these statics are processed?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's the reference on
#[used]
, and the equivalent Clang attribute and GCC attribute.But these are somewhat vague, perhaps intentionally so as this is very much a linker concept? I don't really have a good reference on linkers, the best I can do is reference this piece of source code in
rustc
that talks about a workaround for static libs, and the following section from the manual page forld64
:(Note that Rust
.rlib
s are internally archives / static libraries, and so the rules for static libaries apply to them as well).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if you have more specific questions about how things work then I can try to answer them, to the best of my ability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. How about adding a test to show it work as expected? You can add a dylib crate that allocs in the root directory and then add a test crate that links both the dylib and jemalloc-sys. If it works as expected the test shoud be able to use jemalloc's free to dealloc the pointer from dylib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
I found the closest thing to a reference link, and have rewritten the docs around it to hopefully be clearer.
I have also added two tests:
malloc_and_libc_are_interoperable_when_overridden
, which tests that the overriding actually works on macOS.test-dylib
, which tests that when linking a dylib, the symbol is correctly overridden. Note that I couldn't reproduce it with the current nightly, so something might have changed recently that makes this hack redundant nowadays? Unsure, though it doesn't hurt to have in any case.Failed CI run of the first commit with just the tests: https://github.com/madsmtm/jemallocator/actions/runs/15490515399
Successful CI run after the second commit: https://github.com/madsmtm/jemallocator/actions/runs/15490429305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the
test-dylib
test to not work without this PR, have pushed that as the first commit instead.Failed CI run: https://github.com/madsmtm/jemallocator/actions/runs/17785024568
Succesful CI run: https://github.com/madsmtm/jemallocator/actions/runs/17784954361