Skip to content
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.

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

offset_of: allow (unstably) taking the offset of slice tail fields #126150

Merged
merged 1 commit into from
Jun 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions compiler/rustc_codegen_cranelift/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -832,9 +832,10 @@ fn codegen_stmt<'tcx>(
let val = match null_op {
NullOp::SizeOf => layout.size.bytes(),
NullOp::AlignOf => layout.align.abi.bytes(),
NullOp::OffsetOf(fields) => {
layout.offset_of_subfield(fx, fields.iter()).bytes()
}
NullOp::OffsetOf(fields) => fx
.tcx
.offset_of_subfield(ParamEnv::reveal_all(), layout, fields.iter())
.bytes(),
NullOp::UbChecks => {
let val = fx.tcx.sess.ub_checks();
let val = CValue::by_val(
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx.cx().const_usize(val)
}
mir::NullOp::OffsetOf(fields) => {
let val = layout.offset_of_subfield(bx.cx(), fields.iter()).bytes();
let val = bx
.tcx()
.offset_of_subfield(bx.param_env(), layout, fields.iter())
.bytes();
bx.cx().const_usize(val)
}
mir::NullOp::UbChecks => {
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_const_eval/src/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
Scalar::from_target_usize(val, self)
}
mir::NullOp::OffsetOf(fields) => {
let val = layout.offset_of_subfield(self, fields.iter()).bytes();
let val = self
.tcx
.offset_of_subfield(self.param_env, layout, fields.iter())
.bytes();
Scalar::from_target_usize(val, self)
}
mir::NullOp::UbChecks => Scalar::from_bool(self.tcx.sess.ub_checks()),
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_feature/src/unstable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,8 @@ declare_features! (
(unstable, offset_of_enum, "1.75.0", Some(120141)),
/// Allows using multiple nested field accesses in offset_of!
(unstable, offset_of_nested, "1.77.0", Some(120140)),
/// Allows using fields with slice type in offset_of!
(unstable, offset_of_slice, "CURRENT_RUSTC_VERSION", Some(126151)),
/// Allows using `#[optimize(X)]`.
(unstable, optimize_attribute, "1.34.0", Some(54882)),
/// Allows postfix match `expr.match { ... }`
Expand Down
35 changes: 29 additions & 6 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3363,7 +3363,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let field_ty = self.field_ty(expr.span, field, args);

// FIXME: DSTs with static alignment should be allowed
// Enums are anyway always sized. But just to safeguard against future
// language extensions, let's double-check.
self.require_type_is_sized(field_ty, expr.span, ObligationCauseCode::Misc);

if field.vis.is_accessible_from(sub_def_scope, self.tcx) {
Expand Down Expand Up @@ -3391,8 +3392,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
{
Copy link
Member

Choose a reason for hiding this comment

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

Can't leave an inline comment bc github is dumb, but there's a:

// FIXME: DSTs with static alignment should be allowed

...above for the enum case.

But we don't allow that for enums, since their tails are required to be sized. Can you tweak that message so it doesn't look like something that someone may be interested in fixing, since it's not really actionable unless we were to tweak enums in general.

let field_ty = self.field_ty(expr.span, field, args);

// FIXME: DSTs with static alignment should be allowed
self.require_type_is_sized(field_ty, expr.span, ObligationCauseCode::Misc);
if self.tcx.features().offset_of_slice {
self.require_type_has_static_alignment(
field_ty,
expr.span,
ObligationCauseCode::Misc,
);
} else {
self.require_type_is_sized(
field_ty,
expr.span,
ObligationCauseCode::Misc,
);
}

if field.vis.is_accessible_from(def_scope, self.tcx) {
self.tcx.check_stability(field.did, Some(expr.hir_id), expr.span, None);
Expand All @@ -3412,10 +3424,21 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if let Ok(index) = field.as_str().parse::<usize>()
&& field.name == sym::integer(index)
{
for ty in tys.iter().take(index + 1) {
self.require_type_is_sized(ty, expr.span, ObligationCauseCode::Misc);
}
Comment on lines -3415 to -3417
Copy link
Member

Choose a reason for hiding this comment

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

What's the justification for removing this? Trivial that this is implied by the well-formedness of the type?

For annoying level of completetness, is there a test that exercises this? If not, pls add. Maybe something like offset_of!(([i32], i32), 1)

Copy link
Member Author

@RalfJung RalfJung Jun 8, 2024

Choose a reason for hiding this comment

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

I made sure all tests still pass and couldn't see any reason why the loop existed, and there was no comment explaining it, so I removed it.

We don't have a similar loop for ADTs either. Not sure what is the point of a test for an obviously ill-formed type... and why only for tuples? Such a test, if we want it, is entirely orthogonal for this PR and could cover all sorts of axes of ill-definedness of a type.

EDIT: Turns out we had a good place to put such a test. 🤷

if let Some(&field_ty) = tys.get(index) {
if self.tcx.features().offset_of_slice {
self.require_type_has_static_alignment(
field_ty,
expr.span,
ObligationCauseCode::Misc,
);
} else {
self.require_type_is_sized(
field_ty,
expr.span,
ObligationCauseCode::Misc,
);
}

field_indices.push((FIRST_VARIANT, index.into()));
current_container = field_ty;

Expand Down
20 changes: 20 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

pub fn require_type_has_static_alignment(
&self,
ty: Ty<'tcx>,
span: Span,
code: traits::ObligationCauseCode<'tcx>,
) {
if !ty.references_error() {
let tail =
self.tcx.struct_tail_with_normalize(ty, |ty| self.normalize(span, ty), || {});
// Sized types have static alignment, and so do slices.
if tail.is_trivially_sized(self.tcx) || matches!(tail.kind(), ty::Slice(..)) {
// Nothing else is required here.
} else {
// We can't be sure, let's required full `Sized`.
let lang_item = self.tcx.require_lang_item(LangItem::Sized, None);
self.require_type_meets(ty, span, code, lang_item);
Comment on lines +403 to +404
Copy link
Member

Choose a reason for hiding this comment

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

require_type_is_sized should be a tiny bit more concise

Copy link
Member Author

Choose a reason for hiding this comment

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

That also duplicates some of the logic, though.

}
}
}

pub fn register_bound(
&self,
ty: Ty<'tcx>,
Expand Down
34 changes: 34 additions & 0 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1351,3 +1351,37 @@ pub trait FnAbiOf<'tcx>: FnAbiOfHelpers<'tcx> {
}

impl<'tcx, C: FnAbiOfHelpers<'tcx>> FnAbiOf<'tcx> for C {}

impl<'tcx> TyCtxt<'tcx> {
pub fn offset_of_subfield<I>(
self,
param_env: ty::ParamEnv<'tcx>,
mut layout: TyAndLayout<'tcx>,
indices: I,
) -> Size
where
I: Iterator<Item = (VariantIdx, FieldIdx)>,
{
let cx = LayoutCx { tcx: self, param_env };
let mut offset = Size::ZERO;

for (variant, field) in indices {
layout = layout.for_variant(&cx, variant);
let index = field.index();
offset += layout.fields.offset(index);
layout = layout.field(&cx, index);
if !layout.is_sized() {
// If it is not sized, then the tail must still have at least a known static alignment.
let tail = self.struct_tail_erasing_lifetimes(layout.ty, param_env);
if !matches!(tail.kind(), ty::Slice(..)) {
bug!(
"offset of not-statically-aligned field (type {:?}) cannot be computed statically",
layout.ty
);
}
}
}

offset
}
}
10 changes: 6 additions & 4 deletions compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_middle::bug;
use rustc_middle::mir::interpret::{InterpResult, Scalar};
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::ty::layout::{HasParamEnv, LayoutOf};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_mir_dataflow::value_analysis::{
Map, PlaceIndex, State, TrackElem, ValueAnalysis, ValueAnalysisWrapper, ValueOrPlace,
Expand Down Expand Up @@ -285,9 +285,11 @@ impl<'tcx> ValueAnalysis<'tcx> for ConstAnalysis<'_, 'tcx> {
let val = match null_op {
NullOp::SizeOf if layout.is_sized() => layout.size.bytes(),
NullOp::AlignOf if layout.is_sized() => layout.align.abi.bytes(),
NullOp::OffsetOf(fields) => {
layout.offset_of_subfield(&self.ecx, fields.iter()).bytes()
}
NullOp::OffsetOf(fields) => self
.ecx
.tcx
.offset_of_subfield(self.ecx.param_env(), layout, fields.iter())
.bytes(),
_ => return ValueOrPlace::Value(FlatSet::Top),
};
FlatSet::Elem(Scalar::from_target_usize(val, &self.tcx))
Expand Down
10 changes: 6 additions & 4 deletions compiler/rustc_mir_transform/src/gvn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ use rustc_middle::bug;
use rustc_middle::mir::interpret::GlobalAlloc;
use rustc_middle::mir::visit::*;
use rustc_middle::mir::*;
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::ty::layout::{HasParamEnv, LayoutOf};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_span::def_id::DefId;
use rustc_span::DUMMY_SP;
Expand Down Expand Up @@ -484,9 +484,11 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
let val = match null_op {
NullOp::SizeOf => layout.size.bytes(),
NullOp::AlignOf => layout.align.abi.bytes(),
NullOp::OffsetOf(fields) => {
layout.offset_of_subfield(&self.ecx, fields.iter()).bytes()
}
NullOp::OffsetOf(fields) => self
.ecx
.tcx
.offset_of_subfield(self.ecx.param_env(), layout, fields.iter())
.bytes(),
NullOp::UbChecks => return None,
};
let usize_layout = self.ecx.layout_of(self.tcx.types.usize).unwrap();
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_mir_transform/src/known_panics_lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,9 +625,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let val = match null_op {
NullOp::SizeOf => op_layout.size.bytes(),
NullOp::AlignOf => op_layout.align.abi.bytes(),
NullOp::OffsetOf(fields) => {
op_layout.offset_of_subfield(self, fields.iter()).bytes()
}
NullOp::OffsetOf(fields) => self
.tcx
.offset_of_subfield(self.param_env, op_layout, fields.iter())
.bytes(),
NullOp::UbChecks => return None,
};
ImmTy::from_scalar(Scalar::from_target_usize(val, self), layout).into()
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1304,6 +1304,7 @@ symbols! {
offset_of,
offset_of_enum,
offset_of_nested,
offset_of_slice,
ok_or_else,
omit_gdb_pretty_printer_section,
on,
Expand Down
23 changes: 0 additions & 23 deletions compiler/rustc_target/src/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,29 +256,6 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
Ty::is_transparent(self)
}

pub fn offset_of_subfield<C, I>(self, cx: &C, indices: I) -> Size
where
Ty: TyAbiInterface<'a, C>,
I: Iterator<Item = (VariantIdx, FieldIdx)>,
{
let mut layout = self;
let mut offset = Size::ZERO;

for (variant, field) in indices {
layout = layout.for_variant(cx, variant);
let index = field.index();
offset += layout.fields.offset(index);
layout = layout.field(cx, index);
assert!(
layout.is_sized(),
"offset of unsized field (type {:?}) cannot be computed statically",
layout.ty
);
}

offset
}

/// Finds the one field that is not a 1-ZST.
/// Returns `None` if there are multiple non-1-ZST fields or only 1-ZST-fields.
pub fn non_1zst_field<C>(&self, cx: &C) -> Option<(usize, Self)>
Expand Down
26 changes: 26 additions & 0 deletions tests/ui/feature-gates/feature-gate-offset-of-slice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
use std::mem::offset_of;

struct S {
a: u8,
b: (u8, u8),
c: [i32],
}

struct T {
x: i32,
y: S,
}

type Tup = (i32, [i32]);

fn main() {
offset_of!(S, c); //~ ERROR size for values of type `[i32]` cannot be known at compilation time
}

fn other() {
offset_of!(T, y); //~ ERROR size for values of type `[i32]` cannot be known at compilation time
}

fn tuple() {
offset_of!(Tup, 1); //~ ERROR size for values of type `[i32]` cannot be known at compilation time
}
35 changes: 35 additions & 0 deletions tests/ui/feature-gates/feature-gate-offset-of-slice.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
error[E0277]: the size for values of type `[i32]` cannot be known at compilation time
--> $DIR/feature-gate-offset-of-slice.rs:17:5
|
LL | offset_of!(S, c);
| ^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `Sized` is not implemented for `[i32]`
= note: this error originates in the macro `offset_of` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the size for values of type `[i32]` cannot be known at compilation time
--> $DIR/feature-gate-offset-of-slice.rs:21:5
|
LL | offset_of!(T, y);
| ^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: within `S`, the trait `Sized` is not implemented for `[i32]`, which is required by `S: Sized`
note: required because it appears within the type `S`
--> $DIR/feature-gate-offset-of-slice.rs:3:8
|
LL | struct S {
| ^
= note: this error originates in the macro `offset_of` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the size for values of type `[i32]` cannot be known at compilation time
--> $DIR/feature-gate-offset-of-slice.rs:25:5
|
LL | offset_of!(Tup, 1);
| ^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `Sized` is not implemented for `[i32]`
= note: this error originates in the macro `offset_of` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0277`.
4 changes: 4 additions & 0 deletions tests/ui/offset-of/offset-of-dst-field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,7 @@ fn delta() {
fn generic_with_maybe_sized<T: ?Sized>() -> usize {
offset_of!(Delta<T>, z) //~ ERROR the size for values of type
}

fn illformed_tuple() {
offset_of!(([u8], u8), 1); //~ ERROR the size for values of type
}
11 changes: 10 additions & 1 deletion tests/ui/offset-of/offset-of-dst-field.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ LL - fn generic_with_maybe_sized<T: ?Sized>() -> usize {
LL + fn generic_with_maybe_sized<T>() -> usize {
|

error: aborting due to 8 previous errors
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
--> $DIR/offset-of-dst-field.rs:54:16
|
LL | offset_of!(([u8], u8), 1);
| ^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `Sized` is not implemented for `[u8]`
= note: only the last element of a tuple may have a dynamically sized type

error: aborting due to 9 previous errors

For more information about this error, try `rustc --explain E0277`.
26 changes: 26 additions & 0 deletions tests/ui/offset-of/offset-of-slice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//@run-pass
#![feature(offset_of_slice, offset_of_nested)]

use std::mem::offset_of;

#[repr(C)]
struct S {
a: u8,
b: (u8, u8),
c: [i32],
}

#[repr(C)]
struct T {
x: i8,
y: S,
}

type Tup = (i16, [i32]);

fn main() {
assert_eq!(offset_of!(S, c), 4);
assert_eq!(offset_of!(T, y), 4);
assert_eq!(offset_of!(T, y.c), 8);
assert_eq!(offset_of!(Tup, 1), 4);
}
Loading