From 33ce529e6c31aa63cef5967f5481be291156fb18 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Sun, 17 Aug 2025 13:24:11 +0000 Subject: [PATCH] docs(allocator): correct and expand safety comments on `Allocator` methods (#13139) Correct safety comments on unsafe methods of `Allocator`, as suggested by AI in https://github.com/oxc-project/oxc/pull/13134#pullrequestreview-3125823899. Also expand the safety comments for `Allocator::set_data_ptr`, and add comments on how its invariants are upheld at call sites. Also correct typos and improve comment formatting. --- crates/oxc_allocator/src/from_raw_parts.rs | 26 ++++++++++++++++----- crates/oxc_allocator/src/pool_fixed_size.rs | 2 ++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/crates/oxc_allocator/src/from_raw_parts.rs b/crates/oxc_allocator/src/from_raw_parts.rs index 61a0893a85180..4ecb6d1c05a46 100644 --- a/crates/oxc_allocator/src/from_raw_parts.rs +++ b/crates/oxc_allocator/src/from_raw_parts.rs @@ -139,11 +139,10 @@ impl Allocator { /// describes the start of the allocation obtained from system allocator. /// /// The `Allocator` **must not be allowed to be dropped** or it would be UB. - /// Only use this method if you prevent that possibililty. e.g.: + /// Only use this method if you prevent that possibility. e.g.: /// /// 1. Set the data pointer back to its correct value before it is dropped, using [`set_data_ptr`]. - /// 2. Wrap the `Allocator` in `ManuallyDrop`, and taking care of deallocating it manually - /// with the correct pointer. + /// 2. Wrap the `Allocator` in `ManuallyDrop`, and deallocate its memory manually with the correct pointer. /// /// # Panics /// @@ -182,8 +181,10 @@ impl Allocator { // Set new data pointer. // SAFETY: `Allocator` must have at least 1 allocated chunk or check for sufficient capacity // above would have failed. + // `new_data_ptr` cannot be after the cursor pointer, or free capacity check would have failed. // Data pointer is always aligned on `MIN_ALIGN`, and we rounded `alloc_bytes` up to a multiple // of `MIN_ALIGN`, so that remains the case. + // It is caller's responsibility to ensure that the `Allocator` is not dropped after this call. unsafe { self.set_data_ptr(new_data_ptr) }; // Return original data pointer @@ -204,15 +205,28 @@ impl Allocator { /// It is only here for manually writing data to start of the allocator chunk, /// and then adjusting the start pointer to after it. /// + /// If calling this method with any pointer which is not the original data pointer for this + /// `Allocator` chunk, the `Allocator` must NOT be allowed to be dropped after this call, + /// because data pointer no longer correctly describes the start of the allocation obtained + /// from system allocator. If the `Allocator` were dropped, it'd be UB. + /// + /// Only use this method if you prevent that possibility. e.g.: + /// + /// 1. Set the data pointer back to its correct value before it is dropped, using [`set_data_ptr`]. + /// 2. Wrap the `Allocator` in `ManuallyDrop`, and deallocate its memory manually with the correct pointer. + /// /// # SAFETY /// /// * Allocator must have at least 1 allocated chunk. /// It is UB to call this method on an `Allocator` which has not allocated /// i.e. fresh from `Allocator::new`. - /// * `ptr` must point to within the allocation underlying this allocator. + /// * `ptr` must point to within the `Allocator`'s current chunk. + /// * `ptr` must be equal to or before cursor pointer for this chunk. /// * `ptr` must be aligned on [`RAW_MIN_ALIGN`]. + /// * `Allocator` must not be dropped if `ptr` is not the original data pointer for this chunk. /// /// [`RAW_MIN_ALIGN`]: Self::RAW_MIN_ALIGN + /// [`set_data_ptr`]: Self::set_data_ptr pub unsafe fn set_data_ptr(&self, ptr: NonNull) { debug_assert!((ptr.as_ptr() as usize).is_multiple_of(MIN_ALIGN)); @@ -241,8 +255,8 @@ impl Allocator { /// * Allocator must have at least 1 allocated chunk. /// It is UB to call this method on an `Allocator` which has not allocated /// i.e. fresh from `Allocator::new`. - /// * `ptr` must point to within the allocation underlying this allocator. - /// * `ptr` must be after `data_ptr` for this chunk. + /// * `ptr` must point to within the `Allocator`'s current chunk. + /// * `ptr` must be equal to or after data pointer for this chunk. pub unsafe fn set_cursor_ptr(&self, ptr: NonNull) { // SAFETY: Caller guarantees `Allocator` has at least 1 allocated chunk. // We don't take any action with the `Allocator` while the `&mut ChunkFooter` reference diff --git a/crates/oxc_allocator/src/pool_fixed_size.rs b/crates/oxc_allocator/src/pool_fixed_size.rs index a69fdbe9fd151..8eb0351579a70 100644 --- a/crates/oxc_allocator/src/pool_fixed_size.rs +++ b/crates/oxc_allocator/src/pool_fixed_size.rs @@ -283,6 +283,8 @@ impl FixedSizeAllocator { // SAFETY: Fixed-size allocators have data pointer originally aligned on `BLOCK_ALIGN`, // and size less than `BLOCK_ALIGN`. So we can restore original data pointer by rounding down // to next multiple of `BLOCK_ALIGN`. + // We're restoring the original data pointer, so it cannot break invariants about alignment, + // being within the chunk's allocation, or being before cursor pointer. unsafe { let data_ptr = self.allocator.data_ptr(); let offset = data_ptr.as_ptr() as usize % BLOCK_ALIGN;