SchrodingerZhu / snmalloc-rs

rust bindings of snmalloc
MIT License
122 stars 16 forks source link

removal of libc dependency breaks compilation on systems with older libc #145

Closed mfelsche closed 3 years ago

mfelsche commented 3 years ago

As reported upstream in https://github.com/microsoft/snmalloc/issues/328 snmalloc needs a fix for older libc versions that dont have a getentropy function.

While upgrading snmalloc-rs from 0.2.26 to 0.2.27 we broke our builds as we need to build on an old libc system (2.17 to be exact) (don ask). While 0.2.26 brought its own libc, which was more up to date than the system one, compilation succeeded.

With 0.2.27 we now depend on the system libc, which broke our build.

I am working on a fix for upstream snmalloc to work on older libc systems, but wanted to report this to you anyways.

ryancinsight commented 3 years ago

Not sure we were calling getentropy from snmalloc-rs, that's a newer change on snmalloc side, would be interesting if we could change or override the call from rust though with a wrapper in these situations

mfelsche commented 3 years ago

I am not sure, if a wrapper might solve it, as it is failing at compile time. The call to getentropy in question is here: https://github.com/microsoft/snmalloc/blob/master/src/pal/pal_posix.h#L294

ryancinsight commented 3 years ago

What's your architecture/dockerfile i've been wanting to experiment with this so may try on side while you work on snmalloc side. What I'm thinking is doing something similar to this puts example and similar to ld_preload, I've seen it done with dlpopen calls:

use libc::c_char; use std::ffi::CString;

[no_mangle]

unsafe extern "C" fn puts(s: *const c_char) -> libc::c_int { println!("made some chnge");

let puts_ptr = libc::dlsym(libc::RTLD_NEXT, CString::new("puts").unwrap().into_raw());
let real_puts: fn(*const c_char) -> libc::c_int = std::mem::transmute(puts_ptr);
real_puts(s)

}

mjp41 commented 3 years ago

Not sure we were calling getentropy from snmalloc-rs, that's a newer change on snmalloc side, would be interesting if we could change or override the call from rust though with a wrapper in these situations

It's part of my current changes to add some security features to snmalloc. They are not exercised by the Rust config. I will expose more broadly, when they are more comprehensive.

mfelsche commented 3 years ago

@ryancinsight i used this docker image to reproduce the issue: https://github.com/tremor-rs/tremor-runtime/blob/main/packaging/builder-images/Dockerfile.x86_64-unknown-linux-gnu

Licenser commented 3 years ago

Is there any news on this?

mjp41 commented 3 years ago

@Licenser Does https://github.com/microsoft/snmalloc/pull/329 fix this upstream for you?

mjp41 commented 3 years ago

@SchrodingerZhu I wonder if it makes sense to create a new revision of snmalloc-rs. There are a few upstream changes on master, that would be good to release.

@Licenser I have been doing quite a large refactor over the last 6 months, so the rate of changes to the release is pretty low. Hopefully soon, we will switch to snmalloc2 branch being the release, and then things will change more rapidly.

ryancinsight commented 3 years ago

@mjp41 will you be making snmalloc2 master or have parallel development. Should be possible to allow use of either/or with a flag if they are distinct.

mjp41 commented 3 years ago

@ryancinsight I will probably make it master, and have keep an snmalloc1 branch that I can put critical fixes on. When I have the final piece in place, snmalloc2 should be superior on all fronts. But let's see what the users actually say when were ready. There is one part missing at the moment, so its perf is bad in one case.

mjp41 commented 3 years ago

I will also suggest we do a major version bump for snmalloc-rs when we get there.

SchrodingerZhu commented 3 years ago

I will take a look and make this crate im track with master (snmalloc 1) .

Sent from ProtonMail mobile

-------- Original Message -------- On Oct 7, 2021, 5:34 PM, Matthew Parkinson < @.***> wrote:

@.***SchrodingerZhu I wonder if it makes sense to create a new revision of snmalloc-rs. There are a few upstream changes on master, that would be good to release.

@.***Licenser I have been doing quite a large refactor over the last 6 months, so the rate of changes to the release is pretty low. Hopefully soon, we will switch to snmalloc2 branch being the release, and then things will change more rapidly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.AEZNMJKR2RZNTGRPUCYW44DUFVST5A5CNFSM45GZIFM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOG7RPP2Y.gif

mjp41 commented 3 years ago

Does 0.2.28 address this issue?

Licenser commented 3 years ago

Sorry for the late reply, the last week was busy to say at least, I'll give it a test tomorrow :) (on the bright side, 0.2.28 is measurably faster than prior versions! (see https://www.tremor.rs/benchmarks/ the throughput-logging-json benchmark has quite a significant bump up on the right (2d3013) when merging 0.2.28 in :)

Licenser commented 3 years ago

This seems resolved, yap! Thank you all for the help <3

mjp41 commented 3 years ago

@Licenser that is awesome. And great to see we are going faster.