Skip to content

Don't use Immediate::offset to transmute pointers to integers #131068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Don't use Immediate::offset to transmute pointers to integers #131068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8f9a32
interpret: Immediate::offset: use shared sanity-check function to ens…
RalfJung Sep 30, 2024
7d63efd
fix GVN trying to transmute pointers to integers
RalfJung Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 35 additions & 29 deletions compiler/rustc_const_eval/src/interpret/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,28 +113,47 @@ impl<Prov: Provenance> Immediate<Prov> {
}

/// Assert that this immediate is a valid value for the given ABI.
pub fn assert_matches_abi(self, abi: Abi, cx: &impl HasDataLayout) {
pub fn assert_matches_abi(self, abi: Abi, msg: &str, cx: &impl HasDataLayout) {
match (self, abi) {
(Immediate::Scalar(scalar), Abi::Scalar(s)) => {
assert_eq!(scalar.size(), s.size(cx));
assert_eq!(scalar.size(), s.size(cx), "{msg}: scalar value has wrong size");
if !matches!(s.primitive(), abi::Pointer(..)) {
// This is not a pointer, it should not carry provenance.
assert!(matches!(scalar, Scalar::Int(..)));
assert!(
matches!(scalar, Scalar::Int(..)),
"{msg}: scalar value should be an integer, but has provenance"
);
}
}
(Immediate::ScalarPair(a_val, b_val), Abi::ScalarPair(a, b)) => {
assert_eq!(a_val.size(), a.size(cx));
assert_eq!(
a_val.size(),
a.size(cx),
"{msg}: first component of scalar pair has wrong size"
);
if !matches!(a.primitive(), abi::Pointer(..)) {
assert!(matches!(a_val, Scalar::Int(..)));
assert!(
matches!(a_val, Scalar::Int(..)),
"{msg}: first component of scalar pair should be an integer, but has provenance"
);
}
assert_eq!(b_val.size(), b.size(cx));
assert_eq!(
b_val.size(),
b.size(cx),
"{msg}: second component of scalar pair has wrong size"
);
if !matches!(b.primitive(), abi::Pointer(..)) {
assert!(matches!(b_val, Scalar::Int(..)));
assert!(
matches!(b_val, Scalar::Int(..)),
"{msg}: second component of scalar pair should be an integer, but has provenance"
);
}
}
(Immediate::Uninit, _) => {}
(Immediate::Uninit, _) => {
assert!(abi.is_sized(), "{msg}: unsized immediates are not a thing");
}
_ => {
bug!("value {self:?} does not match ABI {abi:?})",)
bug!("{msg}: value {self:?} does not match ABI {abi:?})",)
}
}
}
Expand Down Expand Up @@ -241,6 +260,7 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {

#[inline(always)]
pub fn from_immediate(imm: Immediate<Prov>, layout: TyAndLayout<'tcx>) -> Self {
// Without a `cx` we cannot call `assert_matches_abi`.
debug_assert!(
match (imm, layout.abi) {
(Immediate::Scalar(..), Abi::Scalar(..)) => true,
Expand All @@ -261,7 +281,6 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {

#[inline]
pub fn from_scalar_int(s: ScalarInt, layout: TyAndLayout<'tcx>) -> Self {
assert_eq!(s.size(), layout.size);
Self::from_scalar(Scalar::from(s), layout)
}

Expand Down Expand Up @@ -334,7 +353,10 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
/// given layout.
// Not called `offset` to avoid confusion with the trait method.
fn offset_(&self, offset: Size, layout: TyAndLayout<'tcx>, cx: &impl HasDataLayout) -> Self {
debug_assert!(layout.is_sized(), "unsized immediates are not a thing");
// Verify that the input matches its type.
if cfg!(debug_assertions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding a debug_assert_matches_abi method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO no, I'd rather not duplicate this logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it look like this, with no logic duplication?

pub fn debug_assert_matches_abi(self, abi: Abi, cx: &impl HasDataLayout) {
    if cfg!(debug_assertions) {
        self.assert_matches_abi(abi, cx);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's what you mean.

Didn't seem worth it to me. 🤷

self.assert_matches_abi(self.layout.abi, "invalid input to Immediate::offset", cx);
}
// `ImmTy` have already been checked to be in-bounds, so we can just check directly if this
// remains in-bounds. This cannot actually be violated since projections are type-checked
// and bounds-checked.
Expand Down Expand Up @@ -368,32 +390,14 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
// the field covers the entire type
_ if layout.size == self.layout.size => {
assert_eq!(offset.bytes(), 0);
assert!(
match (self.layout.abi, layout.abi) {
(Abi::Scalar(l), Abi::Scalar(r)) => l.size(cx) == r.size(cx),
(Abi::ScalarPair(l1, l2), Abi::ScalarPair(r1, r2)) =>
l1.size(cx) == r1.size(cx) && l2.size(cx) == r2.size(cx),
_ => false,
},
"cannot project into {} immediate with equally-sized field {}\nouter ABI: {:#?}\nfield ABI: {:#?}",
self.layout.ty,
layout.ty,
self.layout.abi,
layout.abi,
);
**self
}
// extract fields from types with `ScalarPair` ABI
(Immediate::ScalarPair(a_val, b_val), Abi::ScalarPair(a, b)) => {
assert_matches!(layout.abi, Abi::Scalar(..));
Immediate::from(if offset.bytes() == 0 {
// It is "okay" to transmute from `usize` to a pointer (GVN relies on that).
// So only compare the size.
assert_eq!(layout.size, a.size(cx));
a_val
} else {
assert_eq!(offset, a.size(cx).align_to(b.align(cx).abi));
assert_eq!(layout.size, b.size(cx));
b_val
})
}
Expand All @@ -405,6 +409,8 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
self.layout
),
};
// Ensure the new layout matches the new value.
inner_val.assert_matches_abi(layout.abi, "invalid field type in Immediate::offset", cx);

ImmTy::from_immediate(inner_val, layout)
}
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_const_eval/src/interpret/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,11 @@ where
// Things can ge wrong in quite weird ways when this is violated.
// Unfortunately this is too expensive to do in release builds.
if cfg!(debug_assertions) {
src.assert_matches_abi(local_layout.abi, self);
src.assert_matches_abi(
local_layout.abi,
"invalid immediate for given destination place",
self,
);
}
}
Left(mplace) => {
Expand All @@ -679,7 +683,7 @@ where
) -> InterpResult<'tcx> {
// We use the sizes from `value` below.
// Ensure that matches the type of the place it is written to.
value.assert_matches_abi(layout.abi, self);
value.assert_matches_abi(layout.abi, "invalid immediate for given destination place", self);
// Note that it is really important that the type here is the right one, and matches the
// type things are read at. In case `value` is a `ScalarPair`, we don't do any magic here
// to handle padding properly, which is only correct if we never look at this data with the
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_const_eval/src/interpret/projection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ pub trait Projectable<'tcx, Prov: Provenance>: Sized + std::fmt::Debug {
self.offset_with_meta(offset, OffsetMode::Inbounds, MemPlaceMeta::None, layout, ecx)
}

/// This does an offset-by-zero, which is effectively a transmute. Note however that
/// not all transmutes are supported by all projectables -- specifically, if this is an
/// `OpTy` or `ImmTy`, the new layout must have almost the same ABI as the old one
/// (only changing the `valid_range` is allowed and turning integers into pointers).
fn transmute<M: Machine<'tcx, Provenance = Prov>>(
&self,
layout: TyAndLayout<'tcx>,
Expand Down
30 changes: 23 additions & 7 deletions compiler/rustc_mir_transform/src/gvn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ use rustc_middle::ty::layout::{HasParamEnv, LayoutOf};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_span::DUMMY_SP;
use rustc_span::def_id::DefId;
use rustc_target::abi::{self, Abi, FIRST_VARIANT, FieldIdx, Size, VariantIdx};
use rustc_target::abi::{self, Abi, FIRST_VARIANT, FieldIdx, Primitive, Size, VariantIdx};
use smallvec::SmallVec;
use tracing::{debug, instrument, trace};

Expand Down Expand Up @@ -568,13 +568,29 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
CastKind::Transmute => {
let value = self.evaluated[value].as_ref()?;
let to = self.ecx.layout_of(to).ok()?;
// `offset` for immediates only supports scalar/scalar-pair ABIs,
// so bail out if the target is not one.
// `offset` for immediates generally only supports projections that match the
// type of the immediate. However, as a HACK, we exploit that it can also do
// limited transmutes: it only works between types with the same layout, and
// cannot transmute pointers to integers.
if value.as_mplace_or_imm().is_right() {
match (value.layout.abi, to.abi) {
(Abi::Scalar(..), Abi::Scalar(..)) => {}
(Abi::ScalarPair(..), Abi::ScalarPair(..)) => {}
_ => return None,
let can_transmute = match (value.layout.abi, to.abi) {
(Abi::Scalar(s1), Abi::Scalar(s2)) => {
s1.size(&self.ecx) == s2.size(&self.ecx)
&& !matches!(s1.primitive(), Primitive::Pointer(..))
}
(Abi::ScalarPair(a1, b1), Abi::ScalarPair(a2, b2)) => {
a1.size(&self.ecx) == a2.size(&self.ecx) &&
b1.size(&self.ecx) == b2.size(&self.ecx) &&
// The alignment of the second component determines its offset, so that also needs to match.
b1.align(&self.ecx) == b2.align(&self.ecx) &&
// None of the inputs may be a pointer.
!matches!(a1.primitive(), Primitive::Pointer(..))
&& !matches!(b1.primitive(), Primitive::Pointer(..))
}
_ => false,
};
if !can_transmute {
return None;
}
}
value.offset(Size::ZERO, to, &self.ecx).discard_err()?
Expand Down
8 changes: 4 additions & 4 deletions tests/coverage/closure_macro.cov-map
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,19 @@ Number of file 0 mappings: 6
- Code(Counter(0)) at (prev + 3, 1) to (start + 0, 2)

Function name: closure_macro::main::{closure#0}
Raw bytes (35): 0x[01, 01, 03, 01, 05, 05, 0b, 09, 00, 05, 01, 10, 1c, 03, 21, 05, 04, 11, 01, 27, 02, 03, 11, 00, 16, 00, 00, 17, 00, 1e, 07, 02, 09, 00, 0a]
Raw bytes (35): 0x[01, 01, 03, 01, 05, 05, 0b, 09, 0d, 05, 01, 10, 1c, 03, 21, 05, 04, 11, 01, 27, 02, 03, 11, 00, 16, 0d, 00, 17, 00, 1e, 07, 02, 09, 00, 0a]
Number of files: 1
- file 0 => global file 1
Number of expressions: 3
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
- expression 1 operands: lhs = Counter(1), rhs = Expression(2, Add)
- expression 2 operands: lhs = Counter(2), rhs = Zero
- expression 2 operands: lhs = Counter(2), rhs = Counter(3)
Number of file 0 mappings: 5
- Code(Counter(0)) at (prev + 16, 28) to (start + 3, 33)
- Code(Counter(1)) at (prev + 4, 17) to (start + 1, 39)
- Code(Expression(0, Sub)) at (prev + 3, 17) to (start + 0, 22)
= (c0 - c1)
- Code(Zero) at (prev + 0, 23) to (start + 0, 30)
- Code(Counter(3)) at (prev + 0, 23) to (start + 0, 30)
- Code(Expression(1, Add)) at (prev + 2, 9) to (start + 0, 10)
= (c1 + (c2 + Zero))
= (c1 + (c2 + c3))

8 changes: 4 additions & 4 deletions tests/coverage/closure_macro_async.cov-map
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,19 @@ Number of file 0 mappings: 6
- Code(Counter(0)) at (prev + 3, 1) to (start + 0, 2)

Function name: closure_macro_async::test::{closure#0}::{closure#0}
Raw bytes (35): 0x[01, 01, 03, 01, 05, 05, 0b, 09, 00, 05, 01, 15, 1c, 03, 21, 05, 04, 11, 01, 27, 02, 03, 11, 00, 16, 00, 00, 17, 00, 1e, 07, 02, 09, 00, 0a]
Raw bytes (35): 0x[01, 01, 03, 01, 05, 05, 0b, 09, 0d, 05, 01, 15, 1c, 03, 21, 05, 04, 11, 01, 27, 02, 03, 11, 00, 16, 0d, 00, 17, 00, 1e, 07, 02, 09, 00, 0a]
Number of files: 1
- file 0 => global file 1
Number of expressions: 3
- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
- expression 1 operands: lhs = Counter(1), rhs = Expression(2, Add)
- expression 2 operands: lhs = Counter(2), rhs = Zero
- expression 2 operands: lhs = Counter(2), rhs = Counter(3)
Number of file 0 mappings: 5
- Code(Counter(0)) at (prev + 21, 28) to (start + 3, 33)
- Code(Counter(1)) at (prev + 4, 17) to (start + 1, 39)
- Code(Expression(0, Sub)) at (prev + 3, 17) to (start + 0, 22)
= (c0 - c1)
- Code(Zero) at (prev + 0, 23) to (start + 0, 30)
- Code(Counter(3)) at (prev + 0, 23) to (start + 0, 30)
- Code(Expression(1, Add)) at (prev + 2, 9) to (start + 0, 10)
= (c1 + (c2 + Zero))
= (c1 + (c2 + c3))

Loading