Skip to content

Commit c798baf

Browse files
raoulstrackxraoulstrackx
committed
Addressing reviewer comments
1 parent 5674760 commit c798baf

File tree

6 files changed

+99
-71
lines changed

6 files changed

+99
-71
lines changed

‎src/dlmalloc.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ impl<Sys: System> Dlmalloc<Sys> {
230230
}
231231

232232
pub unsafe fn calloc_must_clear(&self, ptr: *mut u8) -> bool {
233-
!Sys::allocates_zeros(&self.system_allocator) || !Chunk::mmapped(Chunk::from_mem(ptr))
233+
!self.system_allocator.allocates_zeros() || !Chunk::mmapped(Chunk::from_mem(ptr))
234234
}
235235

236236
pub unsafe fn malloc(&mut self, size: usize) -> *mut u8 {
@@ -358,7 +358,7 @@ impl<Sys: System> Dlmalloc<Sys> {
358358
DEFAULT_GRANULARITY,
359359
);
360360

361-
let (tbase, tsize, flags) = Sys::alloc(&self.system_allocator, asize);
361+
let (tbase, tsize, flags) = self.system_allocator.alloc(asize);
362362
if tbase.is_null() {
363363
return tbase;
364364
}

‎src/lib.rs

+50-10
Original file line numberDiff line numberDiff line change
@@ -31,27 +31,26 @@ mod dlmalloc;
3131
mod global;
3232

3333
/// A platform interface
34-
pub trait System: Send {
34+
pub unsafe trait System: Send {
3535
/// Allocates system memory region of at least `size` bytes
3636
/// Returns a triple of `(base, size, flags)` where `base` is a pointer to the beginning of the
3737
/// allocated memory region. `size` is the actual size of the region while `flags` specifies
3838
/// properties of the allocated region. If `EXTERN_BIT` (bit 0) set in flags, then we did not
3939
/// allocate this segment and so should not try to deallocate or merge with others.
40-
unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32);
40+
fn alloc(&self, size: usize) -> (*mut u8, usize, u32);
4141

4242
/// Remaps system memory region at `ptr` with size `oldsize` to a potential new location with
4343
/// size `newsize`. `can_move` indicates if the location is allowed to move to a completely new
4444
/// location, or that it is only allowed to change in size. Returns a pointer to the new
4545
/// location in memory.
46-
unsafe fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool)
47-
-> *mut u8;
46+
fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8;
4847

4948
/// Resizes a memory chunk starting at `ptr` with size `oldsize` to a memory region of
5049
/// `newsize` bytes. Returns `true` if the memory region could be resized.
51-
unsafe fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool;
50+
fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool;
5251

5352
/// Frees an entire memory region. Returns `true` if the operation succeeded
54-
unsafe fn free(&self, ptr: *mut u8, size: usize) -> bool;
53+
fn free(&self, ptr: *mut u8, size: usize) -> bool;
5554

5655
/// Indicates if the platform can release a part of memory. For the `flags` argument, see
5756
/// `System::alloc`
@@ -92,18 +91,59 @@ mod sys;
9291
#[path = "linux.rs"]
9392
mod sys;
9493

95-
impl Dlmalloc {
94+
#[cfg(not(any(target_os = "linux", target_os = "macos", target_arch = "wasm32")))]
95+
pub struct Platform {
96+
_priv: (),
97+
}
98+
99+
#[cfg(not(any(target_os = "linux", target_os = "macos", target_arch = "wasm32")))]
100+
impl Platform {
101+
const fn new() -> Platform {
102+
Platform { _priv: () }
103+
}
104+
}
105+
106+
#[cfg(not(any(target_os = "linux", target_os = "macos", target_arch = "wasm32")))]
107+
unsafe impl System for Platform {
108+
fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
109+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
110+
}
111+
112+
fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8 {
113+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
114+
}
115+
116+
fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
117+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
118+
}
119+
120+
fn free(&self, ptr: *mut u8, size: usize) -> bool {
121+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
122+
}
123+
124+
fn can_release_part(&self, flags: u32) -> bool {
125+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
126+
}
127+
128+
fn allocates_zeros(&self) -> bool {
129+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
130+
}
131+
132+
fn page_size(&self) -> usize {
133+
panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
134+
}
135+
}
136+
137+
impl Dlmalloc<Platform> {
96138
/// Creates a new instance of an allocator
97-
#[cfg(any(target_os = "linux", target_arch = "wasm32", target_os = "macos"))]
98139
pub const fn new() -> Dlmalloc<Platform> {
99140
Dlmalloc(dlmalloc::Dlmalloc::new(Platform::new()))
100141
}
101142
}
102143

103144
impl<S> Dlmalloc<S> {
104145
/// Creates a new instance of an allocator
105-
#[cfg(not(any(target_os = "linux", target_arch = "wasm32", target_os = "macos")))]
106-
pub const fn new(sys_allocator: S) -> Dlmalloc<S> {
146+
pub const fn new_with_platform(sys_allocator: S) -> Dlmalloc<S> {
107147
Dlmalloc(dlmalloc::Dlmalloc::new(sys_allocator))
108148
}
109149
}

‎src/linux.rs

+23-25
Original file line numberDiff line numberDiff line change
@@ -17,49 +17,47 @@ impl Platform {
1717
#[cfg(feature = "global")]
1818
static mut LOCK: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER;
1919

20-
impl System for Platform {
21-
unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
22-
let addr = libc::mmap(
23-
0 as *mut _,
24-
size,
25-
libc::PROT_WRITE | libc::PROT_READ,
26-
libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
27-
-1,
28-
0,
29-
);
20+
unsafe impl System for Platform {
21+
fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
22+
let addr = unsafe {
23+
libc::mmap(
24+
0 as *mut _,
25+
size,
26+
libc::PROT_WRITE | libc::PROT_READ,
27+
libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
28+
-1,
29+
0,
30+
)
31+
};
3032
if addr == libc::MAP_FAILED {
3133
(ptr::null_mut(), 0, 0)
3234
} else {
3335
(addr as *mut u8, size, 0)
3436
}
3537
}
3638

37-
unsafe fn remap(
38-
&self,
39-
ptr: *mut u8,
40-
oldsize: usize,
41-
newsize: usize,
42-
can_move: bool,
43-
) -> *mut u8 {
39+
fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8 {
4440
let flags = if can_move { libc::MREMAP_MAYMOVE } else { 0 };
45-
let ptr = libc::mremap(ptr as *mut _, oldsize, newsize, flags);
41+
let ptr = unsafe { libc::mremap(ptr as *mut _, oldsize, newsize, flags) };
4642
if ptr == libc::MAP_FAILED {
4743
ptr::null_mut()
4844
} else {
4945
ptr as *mut u8
5046
}
5147
}
5248

53-
unsafe fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
54-
let rc = libc::mremap(ptr as *mut _, oldsize, newsize, 0);
55-
if rc != libc::MAP_FAILED {
56-
return true;
49+
fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
50+
unsafe {
51+
let rc = libc::mremap(ptr as *mut _, oldsize, newsize, 0);
52+
if rc != libc::MAP_FAILED {
53+
return true;
54+
}
55+
libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0
5756
}
58-
libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0
5957
}
6058

61-
unsafe fn free(&self, ptr: *mut u8, size: usize) -> bool {
62-
libc::munmap(ptr as *mut _, size) == 0
59+
fn free(&self, ptr: *mut u8, size: usize) -> bool {
60+
unsafe { libc::munmap(ptr as *mut _, size) == 0 }
6361
}
6462

6563
fn can_release_part(&self, _flags: u32) -> bool {

‎src/macos.rs

+17-21
Original file line numberDiff line numberDiff line change
@@ -17,39 +17,35 @@ impl Platform {
1717
#[cfg(feature = "global")]
1818
static mut LOCK: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER;
1919

20-
impl System for Platform {
21-
unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
22-
let addr = libc::mmap(
23-
0 as *mut _,
24-
size,
25-
libc::PROT_WRITE | libc::PROT_READ,
26-
libc::MAP_ANON | libc::MAP_PRIVATE,
27-
-1,
28-
0,
29-
);
20+
unsafe impl System for Platform {
21+
fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
22+
let addr = unsafe {
23+
libc::mmap(
24+
0 as *mut _,
25+
size,
26+
libc::PROT_WRITE | libc::PROT_READ,
27+
libc::MAP_ANON | libc::MAP_PRIVATE,
28+
-1,
29+
0,
30+
)
31+
};
3032
if addr == libc::MAP_FAILED {
3133
(ptr::null_mut(), 0, 0)
3234
} else {
3335
(addr as *mut u8, size, 0)
3436
}
3537
}
3638

37-
unsafe fn remap(
38-
&self,
39-
_ptr: *mut u8,
40-
_oldsize: usize,
41-
_newsize: usize,
42-
_can_move: bool,
43-
) -> *mut u8 {
39+
fn remap(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize, _can_move: bool) -> *mut u8 {
4440
ptr::null_mut()
4541
}
4642

47-
unsafe fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
48-
libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0
43+
fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
44+
unsafe { libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0 }
4945
}
5046

51-
unsafe fn free(&self, ptr: *mut u8, size: usize) -> bool {
52-
libc::munmap(ptr as *mut _, size) == 0
47+
fn free(&self, ptr: *mut u8, size: usize) -> bool {
48+
unsafe { libc::munmap(ptr as *mut _, size) == 0 }
5349
}
5450

5551
fn can_release_part(&self, _flags: u32) -> bool {

‎src/wasm.rs

+5-11
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ impl Platform {
1313
}
1414
}
1515

16-
impl System for Platform {
17-
unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
16+
unsafe impl System for Platform {
17+
fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
1818
let pages = size / self.page_size();
1919
let prev = wasm32::memory_grow(0, pages);
2020
if prev == usize::max_value() {
@@ -27,22 +27,16 @@ impl System for Platform {
2727
)
2828
}
2929

30-
unsafe fn remap(
31-
&self,
32-
_ptr: *mut u8,
33-
_oldsize: usize,
34-
_newsize: usize,
35-
_can_move: bool,
36-
) -> *mut u8 {
30+
fn remap(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize, _can_move: bool) -> *mut u8 {
3731
// TODO: I think this can be implemented near the end?
3832
ptr::null_mut()
3933
}
4034

41-
unsafe fn free_part(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize) -> bool {
35+
fn free_part(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize) -> bool {
4236
false
4337
}
4438

45-
unsafe fn free(&self, _ptr: *mut u8, _size: usize) -> bool {
39+
fn free(&self, _ptr: *mut u8, _size: usize) -> bool {
4640
false
4741
}
4842

‎tests/smoke.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use std::cmp;
77

88
#[test]
99
fn smoke() {
10-
let mut a: Dlmalloc = Dlmalloc::new();
10+
let mut a = Dlmalloc::new();
1111
unsafe {
1212
let ptr = a.malloc(1, 1);
1313
assert!(!ptr.is_null());
@@ -25,7 +25,7 @@ fn smoke() {
2525

2626
#[test]
2727
fn stress() {
28-
let mut a: Dlmalloc = Dlmalloc::new();
28+
let mut a = Dlmalloc::new();
2929
let mut rng = rand::thread_rng();
3030
let mut ptrs = Vec::new();
3131
let max = if cfg!(test_lots) { 1_000_000 } else { 1_000 };

0 commit comments

Comments
 (0)