Skip to content

Commit 33ce529

Browse files
committed
docs(allocator): correct and expand safety comments on Allocator methods (#13139)
Correct safety comments on unsafe methods of `Allocator`, as suggested by AI in #13134 (review). 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.
1 parent 02c779f commit 33ce529

File tree

2 files changed

+22
-6
lines changed

2 files changed

+22
-6
lines changed

crates/oxc_allocator/src/from_raw_parts.rs

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,10 @@ impl Allocator {
139139
/// describes the start of the allocation obtained from system allocator.
140140
///
141141
/// The `Allocator` **must not be allowed to be dropped** or it would be UB.
142-
/// Only use this method if you prevent that possibililty. e.g.:
142+
/// Only use this method if you prevent that possibility. e.g.:
143143
///
144144
/// 1. Set the data pointer back to its correct value before it is dropped, using [`set_data_ptr`].
145-
/// 2. Wrap the `Allocator` in `ManuallyDrop`, and taking care of deallocating it manually
146-
/// with the correct pointer.
145+
/// 2. Wrap the `Allocator` in `ManuallyDrop`, and deallocate its memory manually with the correct pointer.
147146
///
148147
/// # Panics
149148
///
@@ -182,8 +181,10 @@ impl Allocator {
182181
// Set new data pointer.
183182
// SAFETY: `Allocator` must have at least 1 allocated chunk or check for sufficient capacity
184183
// above would have failed.
184+
// `new_data_ptr` cannot be after the cursor pointer, or free capacity check would have failed.
185185
// Data pointer is always aligned on `MIN_ALIGN`, and we rounded `alloc_bytes` up to a multiple
186186
// of `MIN_ALIGN`, so that remains the case.
187+
// It is caller's responsibility to ensure that the `Allocator` is not dropped after this call.
187188
unsafe { self.set_data_ptr(new_data_ptr) };
188189

189190
// Return original data pointer
@@ -204,15 +205,28 @@ impl Allocator {
204205
/// It is only here for manually writing data to start of the allocator chunk,
205206
/// and then adjusting the start pointer to after it.
206207
///
208+
/// If calling this method with any pointer which is not the original data pointer for this
209+
/// `Allocator` chunk, the `Allocator` must NOT be allowed to be dropped after this call,
210+
/// because data pointer no longer correctly describes the start of the allocation obtained
211+
/// from system allocator. If the `Allocator` were dropped, it'd be UB.
212+
///
213+
/// Only use this method if you prevent that possibility. e.g.:
214+
///
215+
/// 1. Set the data pointer back to its correct value before it is dropped, using [`set_data_ptr`].
216+
/// 2. Wrap the `Allocator` in `ManuallyDrop`, and deallocate its memory manually with the correct pointer.
217+
///
207218
/// # SAFETY
208219
///
209220
/// * Allocator must have at least 1 allocated chunk.
210221
/// It is UB to call this method on an `Allocator` which has not allocated
211222
/// i.e. fresh from `Allocator::new`.
212-
/// * `ptr` must point to within the allocation underlying this allocator.
223+
/// * `ptr` must point to within the `Allocator`'s current chunk.
224+
/// * `ptr` must be equal to or before cursor pointer for this chunk.
213225
/// * `ptr` must be aligned on [`RAW_MIN_ALIGN`].
226+
/// * `Allocator` must not be dropped if `ptr` is not the original data pointer for this chunk.
214227
///
215228
/// [`RAW_MIN_ALIGN`]: Self::RAW_MIN_ALIGN
229+
/// [`set_data_ptr`]: Self::set_data_ptr
216230
pub unsafe fn set_data_ptr(&self, ptr: NonNull<u8>) {
217231
debug_assert!((ptr.as_ptr() as usize).is_multiple_of(MIN_ALIGN));
218232

@@ -241,8 +255,8 @@ impl Allocator {
241255
/// * Allocator must have at least 1 allocated chunk.
242256
/// It is UB to call this method on an `Allocator` which has not allocated
243257
/// i.e. fresh from `Allocator::new`.
244-
/// * `ptr` must point to within the allocation underlying this allocator.
245-
/// * `ptr` must be after `data_ptr` for this chunk.
258+
/// * `ptr` must point to within the `Allocator`'s current chunk.
259+
/// * `ptr` must be equal to or after data pointer for this chunk.
246260
pub unsafe fn set_cursor_ptr(&self, ptr: NonNull<u8>) {
247261
// SAFETY: Caller guarantees `Allocator` has at least 1 allocated chunk.
248262
// We don't take any action with the `Allocator` while the `&mut ChunkFooter` reference

crates/oxc_allocator/src/pool_fixed_size.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,8 @@ impl FixedSizeAllocator {
283283
// SAFETY: Fixed-size allocators have data pointer originally aligned on `BLOCK_ALIGN`,
284284
// and size less than `BLOCK_ALIGN`. So we can restore original data pointer by rounding down
285285
// to next multiple of `BLOCK_ALIGN`.
286+
// We're restoring the original data pointer, so it cannot break invariants about alignment,
287+
// being within the chunk's allocation, or being before cursor pointer.
286288
unsafe {
287289
let data_ptr = self.allocator.data_ptr();
288290
let offset = data_ptr.as_ptr() as usize % BLOCK_ALIGN;

0 commit comments

Comments
 (0)