Addressing reviewer comments

alexcrichton · Nov 2, 2020 · 8b0a8ee · 8b0a8ee
1 parent 5674760
commit 8b0a8ee
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 71 deletions.
diff --git a/src/dlmalloc.rs b/src/dlmalloc.rs
@@ -230,7 +230,7 @@ impl<Sys: System> Dlmalloc<Sys> {
     }
 
     pub unsafe fn calloc_must_clear(&self, ptr: *mut u8) -> bool {
-        !Sys::allocates_zeros(&self.system_allocator) || !Chunk::mmapped(Chunk::from_mem(ptr))
+        !self.system_allocator.allocates_zeros() || !Chunk::mmapped(Chunk::from_mem(ptr))
     }
 
     pub unsafe fn malloc(&mut self, size: usize) -> *mut u8 {
@@ -358,7 +358,7 @@ impl<Sys: System> Dlmalloc<Sys> {
             DEFAULT_GRANULARITY,
         );
 
-        let (tbase, tsize, flags) = Sys::alloc(&self.system_allocator, asize);
+        let (tbase, tsize, flags) = self.system_allocator.alloc(asize);
         if tbase.is_null() {
             return tbase;
         }

diff --git a/src/lib.rs b/src/lib.rs
@@ -31,27 +31,26 @@ mod dlmalloc;
 mod global;
 
 /// A platform interface
-pub trait System: Send {
+pub unsafe trait System: Send {
     /// Allocates system memory region of at least `size` bytes
     /// Returns a triple of `(base, size, flags)` where `base` is a pointer to the beginning of the
     /// allocated memory region. `size` is the actual size of the region while `flags` specifies
     /// properties of the allocated region. If `EXTERN_BIT` (bit 0) set in flags, then we did not
     /// allocate this segment and so should not try to deallocate or merge with others.
-    unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32);
+    fn alloc(&self, size: usize) -> (*mut u8, usize, u32);
 
     /// Remaps system memory region at `ptr` with size `oldsize` to a potential new location with
     /// size `newsize`. `can_move` indicates if the location is allowed to move to a completely new
     /// location, or that it is only allowed to change in size. Returns a pointer to the new
     /// location in memory.
-    unsafe fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool)
-        -> *mut u8;
+    fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8;
 
     /// Resizes a memory chunk starting at `ptr` with size `oldsize` to a memory region of
     /// `newsize` bytes. Returns `true` if the memory region could be resized.
-    unsafe fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool;
+    fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool;
 
     /// Frees an entire memory region. Returns `true` if the operation succeeded
-    unsafe fn free(&self, ptr: *mut u8, size: usize) -> bool;
+    fn free(&self, ptr: *mut u8, size: usize) -> bool;
 
     /// Indicates if the platform can release a part of memory. For the `flags` argument, see
     /// `System::alloc`
@@ -92,18 +91,61 @@ mod sys;
 #[path = "linux.rs"]
 mod sys;
 
-impl Dlmalloc {
+#[cfg(not(any(target_os = "linux", target_os = "macos", target_arch = "wasm32")))]
+pub struct Platform {
+    _priv: (),
+}
+
+#[cfg(not(any(target_os = "linux", target_os = "macos", target_arch = "wasm32")))]
+impl Platform {
+    const fn new() -> Platform {
+        Platform {
+            _priv: ()
+        }
+    }
+}
+
+#[cfg(not(any(target_os = "linux", target_os = "macos", target_arch = "wasm32")))]
+unsafe impl System for Platform {
+    fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+
+    fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8 {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+
+    fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+
+    fn free(&self, ptr: *mut u8, size: usize) -> bool {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+
+    fn can_release_part(&self, flags: u32) -> bool {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+
+    fn allocates_zeros(&self) -> bool {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+
+    fn page_size(&self) -> usize {
+        panic!("Please implement the `System` interface and use the `Dlmalloc::new_with_platform` method");
+    }
+}
+
+impl Dlmalloc<Platform> {
     /// Creates a new instance of an allocator
-    #[cfg(any(target_os = "linux", target_arch = "wasm32", target_os = "macos"))]
     pub const fn new() -> Dlmalloc<Platform> {
         Dlmalloc(dlmalloc::Dlmalloc::new(Platform::new()))
     }
 }
 
 impl<S> Dlmalloc<S> {
     /// Creates a new instance of an allocator
-    #[cfg(not(any(target_os = "linux", target_arch = "wasm32", target_os = "macos")))]
-    pub const fn new(sys_allocator: S) -> Dlmalloc<S> {
+    pub const fn new_with_platform(sys_allocator: S) -> Dlmalloc<S> {
         Dlmalloc(dlmalloc::Dlmalloc::new(sys_allocator))
     }
 }

diff --git a/src/linux.rs b/src/linux.rs
@@ -17,49 +17,47 @@ impl Platform {
 #[cfg(feature = "global")]
 static mut LOCK: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER;
 
-impl System for Platform {
-    unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
-        let addr = libc::mmap(
-            0 as *mut _,
-            size,
-            libc::PROT_WRITE | libc::PROT_READ,
-            libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
-            -1,
-            0,
-        );
+unsafe impl System for Platform {
+    fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
+        let addr = unsafe {
+            libc::mmap(
+                0 as *mut _,
+                size,
+                libc::PROT_WRITE | libc::PROT_READ,
+                libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
+                -1,
+                0,
+            )
+        };
         if addr == libc::MAP_FAILED {
             (ptr::null_mut(), 0, 0)
         } else {
             (addr as *mut u8, size, 0)
         }
     }
 
-    unsafe fn remap(
-        &self,
-        ptr: *mut u8,
-        oldsize: usize,
-        newsize: usize,
-        can_move: bool,
-    ) -> *mut u8 {
+    fn remap(&self, ptr: *mut u8, oldsize: usize, newsize: usize, can_move: bool) -> *mut u8 {
         let flags = if can_move { libc::MREMAP_MAYMOVE } else { 0 };
-        let ptr = libc::mremap(ptr as *mut _, oldsize, newsize, flags);
+        let ptr = unsafe { libc::mremap(ptr as *mut _, oldsize, newsize, flags) };
         if ptr == libc::MAP_FAILED {
             ptr::null_mut()
         } else {
             ptr as *mut u8
         }
     }
 
-    unsafe fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
-        let rc = libc::mremap(ptr as *mut _, oldsize, newsize, 0);
-        if rc != libc::MAP_FAILED {
-            return true;
+    fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
+        unsafe {
+            let rc = libc::mremap(ptr as *mut _, oldsize, newsize, 0);
+            if rc != libc::MAP_FAILED {
+                return true;
+            }
+            libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0
         }
-        libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0
     }
 
-    unsafe fn free(&self, ptr: *mut u8, size: usize) -> bool {
-        libc::munmap(ptr as *mut _, size) == 0
+    fn free(&self, ptr: *mut u8, size: usize) -> bool {
+        unsafe { libc::munmap(ptr as *mut _, size) == 0 }
     }
 
     fn can_release_part(&self, _flags: u32) -> bool {

diff --git a/src/macos.rs b/src/macos.rs
@@ -17,39 +17,35 @@ impl Platform {
 #[cfg(feature = "global")]
 static mut LOCK: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER;
 
-impl System for Platform {
-    unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
-        let addr = libc::mmap(
-            0 as *mut _,
-            size,
-            libc::PROT_WRITE | libc::PROT_READ,
-            libc::MAP_ANON | libc::MAP_PRIVATE,
-            -1,
-            0,
-        );
+unsafe impl System for Platform {
+    fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
+        let addr = unsafe {
+            libc::mmap(
+                0 as *mut _,
+                size,
+                libc::PROT_WRITE | libc::PROT_READ,
+                libc::MAP_ANON | libc::MAP_PRIVATE,
+                -1,
+                0,
+            )
+        };
         if addr == libc::MAP_FAILED {
             (ptr::null_mut(), 0, 0)
         } else {
             (addr as *mut u8, size, 0)
         }
     }
 
-    unsafe fn remap(
-        &self,
-        _ptr: *mut u8,
-        _oldsize: usize,
-        _newsize: usize,
-        _can_move: bool,
-    ) -> *mut u8 {
+    fn remap(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize, _can_move: bool) -> *mut u8 {
         ptr::null_mut()
     }
 
-    unsafe fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
-        libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0
+    fn free_part(&self, ptr: *mut u8, oldsize: usize, newsize: usize) -> bool {
+        unsafe { libc::munmap(ptr.offset(newsize as isize) as *mut _, oldsize - newsize) == 0 }
     }
 
-    unsafe fn free(&self, ptr: *mut u8, size: usize) -> bool {
-        libc::munmap(ptr as *mut _, size) == 0
+    fn free(&self, ptr: *mut u8, size: usize) -> bool {
+        unsafe { libc::munmap(ptr as *mut _, size) == 0 }
     }
 
     fn can_release_part(&self, _flags: u32) -> bool {

diff --git a/src/wasm.rs b/src/wasm.rs
@@ -13,8 +13,8 @@ impl Platform {
     }
 }
 
-impl System for Platform {
-    unsafe fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
+unsafe impl System for Platform {
+    fn alloc(&self, size: usize) -> (*mut u8, usize, u32) {
         let pages = size / self.page_size();
         let prev = wasm32::memory_grow(0, pages);
         if prev == usize::max_value() {
@@ -27,22 +27,16 @@ impl System for Platform {
         )
     }
 
-    unsafe fn remap(
-        &self,
-        _ptr: *mut u8,
-        _oldsize: usize,
-        _newsize: usize,
-        _can_move: bool,
-    ) -> *mut u8 {
+    fn remap(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize, _can_move: bool) -> *mut u8 {
         // TODO: I think this can be implemented near the end?
         ptr::null_mut()
     }
 
-    unsafe fn free_part(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize) -> bool {
+    fn free_part(&self, _ptr: *mut u8, _oldsize: usize, _newsize: usize) -> bool {
         false
     }
 
-    unsafe fn free(&self, _ptr: *mut u8, _size: usize) -> bool {
+    fn free(&self, _ptr: *mut u8, _size: usize) -> bool {
         false
     }
 

diff --git a/tests/smoke.rs b/tests/smoke.rs
@@ -7,7 +7,7 @@ use std::cmp;
 
 #[test]
 fn smoke() {
-    let mut a: Dlmalloc = Dlmalloc::new();
+    let mut a = Dlmalloc::new();
     unsafe {
         let ptr = a.malloc(1, 1);
         assert!(!ptr.is_null());
@@ -25,7 +25,7 @@ fn smoke() {
 
 #[test]
 fn stress() {
-    let mut a: Dlmalloc = Dlmalloc::new();
+    let mut a = Dlmalloc::new();
     let mut rng = rand::thread_rng();
     let mut ptrs = Vec::new();
     let max = if cfg!(test_lots) { 1_000_000 } else { 1_000 };