Skip to content

Commit

Permalink
Replace unstable strict_provenance feature with crate
Browse files Browse the repository at this point in the history
**Description**
 - Remove `strict_provenance` and replace with usage of `sptr` crate,
   which is by the same person who authored the strict provenance
   change.
 - Add miri testing section with flags that should enable testing of
   strict provenance.
 - Also fix `perfcnt` and `criterion-perf-events` accidentally being
   added to regular dependencies. Moved those back to dev-dependencies.

**Motivation**
Partial progress towards #20.

Ideally we'd just use the strict_provenance APIs in the standard lib,
but those are still unstable (probably will be for some time).

**Testing Done**
 - `cargo build --all-targets`
 - `cargo test`
 - `cargo miri test`
 - `cargo clippy`
  • Loading branch information
declanvk authored and Declan Kelly committed Nov 19, 2022
1 parent f69e4f2 commit cebc453
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 21 deletions.
5 changes: 4 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ version = "0.1.0"
edition = "2021"
license = "MIT OR Apache-2.0"

[dependencies]
sptr = "0.3.2"

[dependencies.tinyvec]
version = "1.6.0"
features = ["std", "rustc_1_57"]
Expand All @@ -14,7 +17,7 @@ criterion = { version = "0.3.5", features = ["html_reports"] }
dhat = "0.3.0"
rustc-hash = "1.1.0"

[target.'cfg(target_arch = "x86")'.dependencies]
[target.'cfg(target_arch = "x86")'.dev-dependencies]
criterion-perf-events = "0.1.4"
perfcnt = "0.7.2"

Expand Down
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@

- [Documentation for the `main` branch](https://declanvk.github.io/blart/)

## Testing

### Miri

Currently we're using some specific crates (`sptr` and in the future back to `core::ptr::*`) to ensure that we're compatible with [Strict Provenance][sp-issue]. The following `MIRIFLAGS` setup should enable checking to make sure that we're compatible.

```bash
MIRIFLAGS="-Zmiri-strict-provenance -Zmiri-symbolic-alignment-check" cargo miri test
```

I think this is useful because we're doing some pointer times with our tagged pointers implementation, mutating the contents of the pointer to store bits of data.

## Fuzzing

To run the fuzzer I use the command:
Expand Down Expand Up @@ -58,3 +70,5 @@ at your option.
Unless you explicitly state otherwise, any contribution intentionally submitted
for inclusion in the work by you, as defined in the Apache-2.0 license, shall be
dual licensed as above, without any additional terms or conditions.

[sp-issue]: https://github.com/rust-lang/rust/issues/95228
3 changes: 2 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// TODO(#20): Use rust stable distribution, remove usage of nightly features
#![feature(slice_ptr_get, strict_provenance, hasher_prefixfree_extras)]
#![feature(slice_ptr_get, hasher_prefixfree_extras)]
#![allow(unstable_name_collisions)]
#![deny(
missing_docs,
clippy::missing_safety_doc,
Expand Down
2 changes: 2 additions & 0 deletions src/nodes/representation/iterators.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use sptr::Strict;

use super::*;
use std::{
iter::FusedIterator,
Expand Down
2 changes: 2 additions & 0 deletions src/nodes/representation/tests.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use sptr::Strict;

use super::*;
use std::mem;

Expand Down
46 changes: 27 additions & 19 deletions src/tagged_pointer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,23 @@
//! pointed-to type, so that it can store several bits of information. For a
//! type with alignment `A`, the number of available bits is `log_2(A)`.

use std::{fmt, mem::align_of, num::NonZeroUsize, ptr::NonNull};
use std::{fmt, mem::align_of, ptr::NonNull};

use sptr::Strict;

/// A non-null pointer type which carries several bits of metadata.
#[repr(transparent)]
pub struct TaggedPointer<P>(NonNull<P>);

impl<P> TaggedPointer<P> {
/// The ABI-required minimum alignment of the `P` type.
///
/// The maximum alignment of a rust type is 2^29 according to the
/// [reference][align-reference]. On 32-bit and 64-bit platforms that means
/// that it is impossible for the POINTER_MASK to be zero, there must be at
/// least 3 bit present.
///
/// [align-reference]: https://doc.rust-lang.org/reference/type-layout.html#the-alignment-modifiers
pub const ALIGNMENT: usize = align_of::<P>();
/// A mask for data-carrying bits of the pointer.
pub const DATA_MASK: usize = !Self::POINTER_MASK;
Expand Down Expand Up @@ -63,12 +72,12 @@ impl<P> TaggedPointer<P> {
// assumes that null is always a zero value.
let unchecked_ptr = unsafe { NonNull::new_unchecked(pointer) };

let ptr_addr = unchecked_ptr.addr();
let ptr_addr = unchecked_ptr.as_ptr().addr();

// Double-check that there are no existing bits stored in the data-carrying
// positions
assert_eq!(
ptr_addr.get() & Self::DATA_MASK,
ptr_addr & Self::DATA_MASK,
0,
"this pointer was not aligned"
);
Expand Down Expand Up @@ -98,18 +107,14 @@ impl<P> TaggedPointer<P> {
/// This pointer is guaranteed to be non-null.
pub fn to_ptr(self) -> *mut P {
self.0
.map_addr(|ptr_addr| {
// SAFETY: The result is guaranteed to be non-zero because this tagged pointer
// was created with the same non-zero value.
unsafe { NonZeroUsize::new_unchecked(ptr_addr.get() & Self::POINTER_MASK) }
})
.as_ptr()
.map_addr(|ptr_addr| ptr_addr & Self::POINTER_MASK)
}

/// Consume this tagged pointer and produce the data it carries.
pub fn to_data(self) -> usize {
let ptr_addr = self.0.addr();
ptr_addr.get() & Self::DATA_MASK
let ptr_addr = self.0.as_ptr().addr();
ptr_addr & Self::DATA_MASK
}

/// Update the data this tagged pointer carries to a new value.
Expand All @@ -125,15 +130,18 @@ impl<P> TaggedPointer<P> {
);
let data = data & Self::DATA_MASK;

self.0 = self.0.map_addr(|ptr_addr| {
let new_addr = (ptr_addr.get() & Self::POINTER_MASK) | data;

// clear the data bits and then write the new data
// SAFETY: This value will always be non-zero because the upper bits (from
// POINTER_MASK) will always be non-zero. This a property of the type and its
// construction.
unsafe { NonZeroUsize::new_unchecked(new_addr) }
let ptr_with_new_data = self.0.as_ptr().map_addr(|ptr_addr| {
(ptr_addr & Self::POINTER_MASK) | data
});

// The `ptr_with_new_data` is guaranteed to be non-null because it's pointer
// address was derived from a non-null pointer using operations that would not
// have zeroed out the address.
//
// The bit-and operation combining the original pointer address (non-null)
// combined with the POINTER_MASK would not have produced a zero value because
// the POINTER_MASK value must have the high bits set.
self.0 = unsafe { NonNull::new_unchecked(ptr_with_new_data) };
}

/// Casts to a [`TaggedPointer`] of another type.
Expand Down Expand Up @@ -423,7 +431,7 @@ mod tests {
0b11111111_11111111_11111111_11111111_11111111_11111111_11111111_11111000usize
);

// Something weird about the representation of u128 on x86 vs aarch64 platforms:
// Something weird about the representation of u128 on x86 vs aarch64 platforms:
// https://github.com/rust-lang/rust/issues/54341
if cfg!(target_arch = "x86") {
assert_eq!(TaggedPointer::<u128>::ALIGNMENT, 8);
Expand Down

0 comments on commit cebc453

Please sign in to comment.