From 56e96fb1b33d6a53dbc6b3df09d3e3f1b70db4af Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sat, 30 Apr 2022 19:44:42 -0700 Subject: [PATCH 1/4] Eliminate ptr->int->ptr casting from Identifier implementation --- build.rs | 6 --- src/backport.rs | 12 ----- src/identifier.rs | 123 ++++++++++++++++++++++++++++++---------------- 3 files changed, 82 insertions(+), 59 deletions(-) diff --git a/build.rs b/build.rs index b39468d2..b45af7e2 100644 --- a/build.rs +++ b/build.rs @@ -8,12 +8,6 @@ fn main() { None => return, }; - if compiler < 32 { - // u64::from_ne_bytes. - // https://doc.rust-lang.org/std/primitive.u64.html#method.from_ne_bytes - println!("cargo:rustc-cfg=no_from_ne_bytes"); - } - if compiler < 33 { // Exhaustive integer patterns. On older compilers, a final `_` arm is // required even if every possible integer value is otherwise covered. diff --git a/src/backport.rs b/src/backport.rs index f3a69c95..c7751b29 100644 --- a/src/backport.rs +++ b/src/backport.rs @@ -14,18 +14,6 @@ impl StripPrefixExt for str { } } -#[cfg(no_from_ne_bytes)] // rustc <1.32 -pub(crate) trait FromNeBytes { - fn from_ne_bytes(bytes: [u8; 8]) -> Self; -} - -#[cfg(no_from_ne_bytes)] -impl FromNeBytes for u64 { - fn from_ne_bytes(bytes: [u8; 8]) -> Self { - unsafe { std::mem::transmute(bytes) } - } -} - pub(crate) use crate::alloc::vec::Vec; #[cfg(no_alloc_crate)] // rustc <1.36 diff --git a/src/identifier.rs b/src/identifier.rs index 500239e7..820f5d47 100644 --- a/src/identifier.rs +++ b/src/identifier.rs @@ -67,40 +67,52 @@ // allows size_of::() == size_of::>(). use crate::alloc::alloc::{alloc, dealloc, Layout}; -#[cfg(no_from_ne_bytes)] -use crate::backport::FromNeBytes; use core::mem; use core::num::{NonZeroU64, NonZeroUsize}; -use core::ptr; +use core::ptr::{self, NonNull}; use core::slice; use core::str; -#[repr(transparent)] +const PTR_BYTES: usize = mem::size_of::>(); + +// If pointers are already 8 bytes or bigger, then 0. If pointers are smaller +// than 8 bytes, then Identifier will contain a byte array to raise its size up +// to 8 bytes total. +const TAIL_BYTES: usize = 8 * (PTR_BYTES < 8) as usize - PTR_BYTES * (PTR_BYTES < 8) as usize; + +#[repr(C, align(8))] pub(crate) struct Identifier { - repr: NonZeroU64, + head: NonNull, + tail: [u8; TAIL_BYTES], } impl Identifier { - const EMPTY: NonZeroU64 = unsafe { NonZeroU64::new_unchecked(!0) }; - pub(crate) const fn empty() -> Self { + // This is a separate constant because unsafe function calls are not + // allowed in a const fn body, only in a const, until later rustc than + // what we support. + const HEAD: NonNull = unsafe { NonNull::new_unchecked(!0 as *mut u8) }; + // `mov rax, -1` - Identifier { repr: Self::EMPTY } + Identifier { + head: HEAD, + tail: [!0; TAIL_BYTES], + } } // SAFETY: string must be ASCII and not contain \0 bytes. pub(crate) unsafe fn new_unchecked(string: &str) -> Self { let len = string.len(); - let repr = match len as u64 { - 0 => Self::EMPTY, + match len as u64 { + 0 => Self::empty(), 1..=8 => { - let mut bytes = [0u8; 8]; + let mut bytes = [0u8; mem::size_of::()]; // SAFETY: string is big enough to read len bytes, bytes is big // enough to write len bytes, and they do not overlap. unsafe { ptr::copy_nonoverlapping(string.as_ptr(), bytes.as_mut_ptr(), len) }; - // SAFETY: it's nonzero because the input string was at least 1 - // byte of ASCII and did not contain \0. - unsafe { NonZeroU64::new_unchecked(u64::from_ne_bytes(bytes)) } + // SAFETY: the head field is nonzero because the input string + // was at least 1 byte of ASCII and did not contain \0. + unsafe { mem::transmute::<[u8; mem::size_of::()], Identifier>(bytes) } } 9..=0xff_ffff_ffff_ffff => { // SAFETY: len is in a range that does not contain 0. @@ -124,25 +136,33 @@ impl Identifier { // SAFETY: size is bytes_for_varint(len) bytes + len bytes. This // is writing to the last len bytes. unsafe { ptr::copy_nonoverlapping(string.as_ptr(), write, len) }; - ptr_to_repr(ptr) + Identifier { + head: ptr_to_repr(ptr), + tail: [0; TAIL_BYTES], + } } 0x100_0000_0000_0000..=0xffff_ffff_ffff_ffff => { unreachable!("please refrain from storing >64 petabytes of text in semver version"); } #[cfg(no_exhaustive_int_match)] // rustc <1.33 _ => unreachable!(), - }; - Identifier { repr } + } } pub(crate) fn is_empty(&self) -> bool { // `cmp rdi, -1` -- basically: `repr as i64 == -1` - self.repr == Self::EMPTY + let empty = Self::empty(); + let is_empty = self.head == empty.head && self.tail == empty.tail; + // The empty representation does nothing on Drop. We can't let this one + // drop normally because `impl Drop for Identifier` calls is_empty; that + // would be an infinite recursion. + mem::forget(empty); + is_empty } fn is_inline(&self) -> bool { // `test rdi, rdi` -- basically: `repr as i64 >= 0` - self.repr.get() >> 63 == 0 + self.head.as_ptr() as usize >> (PTR_BYTES * 8 - 1) == 0 } fn is_empty_or_inline(&self) -> bool { @@ -155,20 +175,23 @@ impl Identifier { "" } else if self.is_inline() { // SAFETY: repr is in the inline representation. - unsafe { inline_as_str(&self.repr) } + unsafe { inline_as_str(self) } } else { // SAFETY: repr is in the heap allocated representation. - unsafe { ptr_as_str(&self.repr) } + unsafe { ptr_as_str(&self.head) } } } } impl Clone for Identifier { fn clone(&self) -> Self { - let repr = if self.is_empty_or_inline() { - self.repr + if self.is_empty_or_inline() { + Identifier { + head: self.head, + tail: self.tail, + } } else { - let ptr = repr_to_ptr(self.repr); + let ptr = repr_to_ptr(self.head); // SAFETY: ptr is one of our own heap allocations. let len = unsafe { decode_len(ptr) }; let size = bytes_for_varint(len) + len.get(); @@ -184,9 +207,11 @@ impl Clone for Identifier { // not a realloc). The argument ptrs are readable/writeable // respectively for size bytes. unsafe { ptr::copy_nonoverlapping(ptr, clone, size) } - ptr_to_repr(clone) - }; - Identifier { repr } + Identifier { + head: ptr_to_repr(clone), + tail: [0; TAIL_BYTES], + } + } } } @@ -195,7 +220,7 @@ impl Drop for Identifier { if self.is_empty_or_inline() { return; } - let ptr = repr_to_ptr_mut(self.repr); + let ptr = repr_to_ptr_mut(self.head); // SAFETY: ptr is one of our own heap allocations. let len = unsafe { decode_len(ptr) }; let size = bytes_for_varint(len) + len.get(); @@ -214,12 +239,12 @@ impl PartialEq for Identifier { fn eq(&self, rhs: &Self) -> bool { if self.is_empty_or_inline() { // Fast path (most common) - self.repr == rhs.repr + self.head == rhs.head && self.tail == rhs.tail } else if rhs.is_empty_or_inline() { false } else { // SAFETY: both reprs are in the heap allocated representation. - unsafe { ptr_as_str(&self.repr) == ptr_as_str(&rhs.repr) } + unsafe { ptr_as_str(&self.head) == ptr_as_str(&rhs.head) } } } } @@ -228,32 +253,48 @@ impl PartialEq for Identifier { // insignificant 0 in the least significant bit. We take advantage of that // unneeded bit to rotate a 1 into the most significant bit to make the repr // distinguishable from ASCII bytes. -fn ptr_to_repr(ptr: *mut u8) -> NonZeroU64 { +fn ptr_to_repr(original: *mut u8) -> NonNull { // `mov eax, 1` // `shld rax, rdi, 63` - let repr = (ptr as u64 | 1).rotate_right(1); + let modified = (original as usize | 1).rotate_right(1); + + // `original + (modified - original)`, but being mindful of provenance. + let diff = modified.wrapping_sub(original as usize); + let modified = original.wrapping_add(diff); // SAFETY: the most significant bit of repr is known to be set, so the value // is not zero. - unsafe { NonZeroU64::new_unchecked(repr) } + unsafe { NonNull::new_unchecked(modified) } } // Shift out the 1 previously placed into the most significant bit of the least // significant byte. Shift in a low 0 bit to reconstruct the original 2-byte // aligned pointer. -fn repr_to_ptr(repr: NonZeroU64) -> *const u8 { +fn repr_to_ptr(modified: NonNull) -> *const u8 { // `lea rax, [rdi + rdi]` - (repr.get() << 1) as *const u8 + let modified = modified.as_ptr(); + let original = (modified as usize) << 1; + + // `modified + (original - modified)`, but being mindful of provenance. + let diff = original.wrapping_sub(modified as usize); + modified.wrapping_add(diff) } -fn repr_to_ptr_mut(repr: NonZeroU64) -> *mut u8 { +fn repr_to_ptr_mut(repr: NonNull) -> *mut u8 { repr_to_ptr(repr) as *mut u8 } // Compute the length of the inline string, assuming the argument is in short // string representation. Short strings are stored as 1 to 8 nonzero ASCII // bytes, followed by \0 padding for the remaining bytes. -fn inline_len(repr: NonZeroU64) -> NonZeroUsize { +// +// SAFETY: the identifier must indeed be in the inline representation. +unsafe fn inline_len(repr: &Identifier) -> NonZeroUsize { + // SAFETY: Identifier's layout is align(8) and at least size 8. We're doing + // an aligned read of the first 8 bytes from it. The bytes are not all zero + // because inline strings are at least 1 byte long and cannot contain \0. + let repr = unsafe { ptr::read(repr as *const Identifier as *const NonZeroU64) }; + // Rustc >=1.53 has intrinsics for counting zeros on a non-zeroable integer. // On many architectures these are more efficient than counting on ordinary // zeroable integers (bsf vs cttz). On rustc <1.53 without those intrinsics, @@ -275,9 +316,9 @@ fn inline_len(repr: NonZeroU64) -> NonZeroUsize { // SAFETY: repr must be in the inline representation, i.e. at least 1 and at // most 8 nonzero ASCII bytes padded on the end with \0 bytes. -unsafe fn inline_as_str(repr: &NonZeroU64) -> &str { - let ptr = repr as *const NonZeroU64 as *const u8; - let len = inline_len(*repr).get(); +unsafe fn inline_as_str(repr: &Identifier) -> &str { + let ptr = repr as *const Identifier as *const u8; + let len = unsafe { inline_len(repr) }.get(); // SAFETY: we are viewing the nonzero ASCII prefix of the inline repr's // contents as a slice of bytes. Input/output lifetimes are correctly // associated. @@ -335,7 +376,7 @@ unsafe fn decode_len(ptr: *const u8) -> NonZeroUsize { // SAFETY: repr must be in the heap allocated representation, with varint header // and string contents already written. -unsafe fn ptr_as_str(repr: &NonZeroU64) -> &str { +unsafe fn ptr_as_str(repr: &NonNull) -> &str { let ptr = repr_to_ptr(*repr); let len = unsafe { decode_len(ptr) }; let header = bytes_for_varint(len); From e245e56a885b21058ed820d93096bd71ec6e41ef Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 1 May 2022 12:45:56 -0700 Subject: [PATCH 2/4] Add miri CI on 32-bit and little and big endian --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 650f13f9..7190c88f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,6 +53,8 @@ jobs: - uses: actions/checkout@v3 - uses: dtolnay/rust-toolchain@miri - run: cargo miri test + - run: cargo miri test --target i686-unknown-linux-gnu + - run: cargo miri test --target mips-unknown-linux-gnu outdated: name: Outdated From de3fbcf013869f54a8e349baa5ccce535defdd1b Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 1 May 2022 12:59:34 -0700 Subject: [PATCH 3/4] Track raw pointers in miri CI run --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7190c88f..f33f3b32 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,8 @@ jobs: miri: name: Miri runs-on: ubuntu-latest + env: + MIRIFLAGS: -Zmiri-tag-raw-pointers steps: - uses: actions/checkout@v3 - uses: dtolnay/rust-toolchain@miri From 1cef40eda32e28223feaa575785c01261f9860be Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 1 May 2022 13:04:21 -0700 Subject: [PATCH 4/4] Comment the endianness of the miri runs --- .github/workflows/ci.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f33f3b32..61657f52 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -55,8 +55,10 @@ jobs: - uses: actions/checkout@v3 - uses: dtolnay/rust-toolchain@miri - run: cargo miri test - - run: cargo miri test --target i686-unknown-linux-gnu - - run: cargo miri test --target mips-unknown-linux-gnu + - name: Run cargo miri test (32-bit little endian) + run: cargo miri test --target i686-unknown-linux-gnu + - name: Run cargo miri test (32-bit big endian) + run: cargo miri test --target mips-unknown-linux-gnu outdated: name: Outdated