Skip to content

Conversation

madsmtm
Copy link

@madsmtm madsmtm commented Dec 15, 2024

The goal is to move the various workarounds for this feature in rustc to the jemalloc-sys crate instead.

I'm not entirely sure of the history here, but it is possible this was not done in the past because #[used] used to not work in libraries, see rust-lang/rust#95604 and rust-lang/rust#133491? In any case, we should be able to do it now.

Copy link

ti-chi-bot bot commented Dec 15, 2024

Welcome @madsmtm! It looks like this is your first PR to tikv/jemallocator 🎉

use super::*;

#[used]
static USED_MALLOC: unsafe extern "C" fn(usize) -> *mut c_void = malloc;
Copy link
Member

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?

Copy link
Author

@madsmtm madsmtm Dec 16, 2024

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 for ld64:

A static library (aka static archive) is a collection of .o files with a table of contents that lists the global symbols in the .o files. ld will only pull .o files out of a static library if needed to resolve some symbol reference. Unlike traditional linkers, ld will continually search a static library while linking.

(Note that Rust .rlibs are internally archives / static libraries, and so the rules for static libaries apply to them as well).

Copy link
Author

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?

Copy link
Member

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.

Copy link
Author

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:

  1. malloc_and_libc_are_interoperable_when_overridden, which tests that the overriding actually works on macOS.
  2. 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

Copy link
Author

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

@madsmtm madsmtm force-pushed the used-unprefixed branch 3 times, most recently from 070e78b to c6670be Compare June 6, 2025 12:31
@madsmtm madsmtm force-pushed the used-unprefixed branch from c6670be to 9bc0fd9 Compare June 6, 2025 12:31
@madsmtm madsmtm requested a review from BusyJay June 6, 2025 12:48
@madsmtm
Copy link
Author

madsmtm commented Sep 16, 2025

Ping @BusyJay, could you take a look? This would be nice to get in, it'd simplify rustc's main function quite a bit.

target_vendor = "apple"
))]
#[used]
static USED_ZONE_REGISTER: unsafe extern "C" fn() = {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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 unprefixed_malloc_on_supported_platforms just plain didn't work on macOS before (unless you inserted these statics yourself like rustc), and now it does.

Or do you fear this will have a chance of breaking something for users?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unprefixed_malloc_on_supported_platforms just plain didn't work on macOS before

It actually works if unprefixed_malloc_on_supported_platforms is interpreted as its name instead of overriding system allocator. The symbol is still unprefixed on MacOS in the past and future.

Or do you fear this will have a chance of breaking something for users?

Yes, it's known not to override system allocator on MacOS.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. In the last commit, I've changed it to:

unprefixed_malloc_on_supported_platforms = []
override = ["unprefixed_malloc_on_supported_platforms"]

I've also made the statics only get emitted with that feature enabled.

- A test that checks that jemalloc's malloc and libc's free are
  interoperable when unprefixed.
- A test that checks that using unprefixed with a shared library still
  overrides the symbols in the shared library.

Signed-off-by: Mads Marquart <[email protected]>
@madsmtm
Copy link
Author

madsmtm commented Sep 17, 2025

I tested this a bit more thoroughly together with mimalloc in https://github.com/madsmtm/test-malloc-overriding btw, have updated the test included in this PR based on that.

@BusyJay
Copy link
Member

BusyJay commented Sep 22, 2025

Rest LGTM

@madsmtm madsmtm force-pushed the used-unprefixed branch 2 times, most recently from 25dd274 to 82b4a4e Compare September 22, 2025 10:28
@madsmtm
Copy link
Author

madsmtm commented Sep 22, 2025

I think I've fixed the things you requested now

@madsmtm madsmtm requested a review from BusyJay September 22, 2025 10:29
@madsmtm
Copy link
Author

madsmtm commented Sep 22, 2025

Fixed CI

@madsmtm madsmtm requested a review from BusyJay September 22, 2025 11:55
@BusyJay BusyJay merged commit 925d696 into tikv:main Sep 23, 2025
10 checks passed
@BusyJay
Copy link
Member

BusyJay commented Sep 23, 2025

Thank you!

@madsmtm madsmtm deleted the used-unprefixed branch September 24, 2025 19:49
@madsmtm
Copy link
Author

madsmtm commented Oct 7, 2025

Would it be possible to get this published as v0.6.1? I can make a PR for it if you want?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants