Skip to content

Commit

Permalink
Fix the issue with TypeId alignment on new Rust Apple Silicon
Browse files Browse the repository at this point in the history
Summary:
* `TypeId` is `u128` [in Rust 1.72](rust-lang/rust#109953)
* `u128` is 16 bytes aligned on Apple Silicon
* we require 8 byte alignment for `StarlarkValue`

Maybe we should fix heap allocations: insert padding to allow any alignment.

But practically we don't need it and should not do it: generated machine code is identical for aligned and 8-byte aligned `u128`: [compiler explorer](https://rust.godbolt.org/z/96KsnjW77) (note it is identical if we load it from memory not pass by value, but this is what we do).

So for now just force 8 byte alignment on `TypeId`.

Fixes issue #408

Reviewed By: ndmitchell, shayne-fletcher

Differential Revision: D49019964

fbshipit-source-id: 62c8d7db83e9e4bdbe58fe7a62d5daae41996e61
  • Loading branch information
stepancheg authored and facebook-github-bot committed Sep 6, 2023
1 parent cdd7ebc commit cff25e4
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 3 deletions.
30 changes: 30 additions & 0 deletions starlark-rust/starlark/src/values/starlark_type_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,33 @@ impl StarlarkTypeId {
StarlarkTypeId(ConstTypeId::of::<T::StaticType>())
}
}

/// We require alignment 8 for `StarlarkValue`.
/// `TypeId` is 16 bytes aligned on Rust 1.72 on Apple Silicon.
/// Use this struct to put `ConstTypeId` in a `StarlarkValue`.
// TODO(nga): remove alignment requirement from `Heap`.
#[repr(packed(8))]
#[derive(Allocative, Eq, Clone, Copy, Dupe, Debug)]
#[allocative(skip)] // There are no heap allocations in this struct.
pub(crate) struct StarlarkTypeIdAligned {
starlark_type_id: StarlarkTypeId,
}

impl PartialEq for StarlarkTypeIdAligned {
#[inline]
fn eq(&self, other: &Self) -> bool {
self.get() == other.get()
}
}

impl StarlarkTypeIdAligned {
#[inline]
pub(crate) const fn new(starlark_type_id: StarlarkTypeId) -> StarlarkTypeIdAligned {
StarlarkTypeIdAligned { starlark_type_id }
}

#[inline]
pub(crate) const fn get(&self) -> StarlarkTypeId {
self.starlark_type_id
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use crate::values::dict::DictRef;
use crate::values::list::value::FrozenList;
use crate::values::list::ListRef;
use crate::values::starlark_type_id::StarlarkTypeId;
use crate::values::starlark_type_id::StarlarkTypeIdAligned;
use crate::values::tuple::value::Tuple;
use crate::values::types::int_or_big::StarlarkIntRef;
use crate::values::typing::type_compiled::matcher::TypeMatcher;
Expand Down Expand Up @@ -245,20 +246,20 @@ impl TypeMatcher for IsNone {

#[derive(Allocative, Debug, Clone)]
pub(crate) struct StarlarkTypeIdMatcher {
starlark_type_id: StarlarkTypeId,
starlark_type_id: StarlarkTypeIdAligned,
}

impl StarlarkTypeIdMatcher {
pub(crate) fn new(ty: TyStarlarkValue) -> StarlarkTypeIdMatcher {
StarlarkTypeIdMatcher {
starlark_type_id: ty.starlark_type_id(),
starlark_type_id: StarlarkTypeIdAligned::new(ty.starlark_type_id()),
}
}
}

impl TypeMatcher for StarlarkTypeIdMatcher {
fn matches(&self, value: Value) -> bool {
value.starlark_type_id() == self.starlark_type_id
value.starlark_type_id() == self.starlark_type_id.get()
}
}

Expand Down

0 comments on commit cff25e4

Please sign in to comment.