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.

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

Jump to bottom

[WIP] Move pointee_info_at to TyLayoutMethods. #57150

Closed
wants to merge 7 commits into from
Closed

[WIP] Move pointee_info_at to TyLayoutMethods. #57150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
15955b9
Move pointee_info_at to TyLayoutMethods.
wildarch Dec 24, 2018
659ffb2
Fix typo in src/librustc/ty/layout.rs
oli-obk Dec 28, 2018
3d3d64f
Return instead of collecting to mut result.
wildarch Dec 28, 2018
e08d9cf
Remove old pointee_info_at body.
wildarch Dec 28, 2018
7cba55e
Add param_env parameter to pointee_info_at.
wildarch Dec 28, 2018
bb05cb8
Make line fit within 100 character limit.
wildarch Jan 11, 2019
3ce7a68
impl `pointee_info_at` in TyLayout.
wildarch Mar 11, 2019
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
136 changes: 136 additions & 0 deletions src/librustc/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ use std::iter;
use std::mem;
use std::ops::Bound;

use hir;

use ich::StableHashingContext;
use rustc_data_structures::indexed_vec::{IndexVec, Idx};
use rustc_data_structures::stable_hasher::{HashStable, StableHasher,
Expand Down Expand Up @@ -1497,6 +1499,7 @@ impl<'gcx, 'tcx, T: HasTyCtxt<'gcx>> HasTyCtxt<'gcx> for LayoutCx<'tcx, T> {
pub trait MaybeResult<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I realize now that this is actually an isomorphism with Result<T, Self::Err>.
In the case of impl<T> MaybeResult<T> for T, that Err associated type would be !, which would make both conversions total.

So the existing API could be replaced with:

  • MaybeResult::from_ok(x) -> MaybeResult::from(Ok(x))
  • x.map_same(f) -> MaybeResult::from(x.to_result().map(f))
  • x.ok() -> x.to_result().ok()

Copy link
Contributor

@saleemjaffer saleemjaffer Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@eddyb I am a bit confused here. Does this mean I have to create two methods from and to_result for MaybeResult?

And in

x.map_same(f) -> MaybeResult::from(x.to_result().map(f))

In this x should have a to_result method. But in our case TyLayout has no such method.

Copy link
Contributor

Choose a reason for hiding this comment

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

from should be a std::convert::From impl.

to_result should be a method, yes.

x.map_same(f) -> MaybeResult::from(x.to_result().map(f))
In this x should have a to_result method. But in our case TyLayout has no such method.

x is a MaybeResult, so it will have the to_result method.

Copy link
Contributor

Choose a reason for hiding this comment

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

@oli-obk I am trying to implement std::convert::From for MaybeResult like this:

impl From<TyLayout<'tcx>> for dyn MaybeResult<TyLayout<'tcx>, Item=!> {
    fn from(ty: TyLayout<'tcx>) -> dyn MaybeResult<TyLayout<'tcx>, Item=!> {
        ty
    }
}

Getting some issues:

error[E0277]: the size for values of type `(dyn ty::layout::MaybeResult<rustc_target::abi::TyLayout<'tcx, &'tcx ty::TyS<'tcx>>, Item = !> + 'static)` cannot be known at compilation time
    --> src/librustc/ty/layout.rs:1495:6
     |
1495 | impl From<TyLayout<'tcx>> for dyn MaybeResult<TyLayout<'tcx>, Item=!> {
     |      ^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
     |
     = help: the trait `std::marker::Sized` is not implemented for `(dyn ty::layout::MaybeResult<rustc_target::abi::TyLayout<'tcx, &'tcx ty::TyS<'tcx>>, Item = !> + 'static)`
     = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>

error[E0038]: the trait `ty::layout::MaybeResult` cannot be made into an object
    --> src/librustc/ty/layout.rs:1495:6
     |
1495 | impl From<TyLayout<'tcx>> for dyn MaybeResult<TyLayout<'tcx>, Item=!> {
     |      ^^^^^^^^^^^^^^^^^^^^ the trait `ty::layout::MaybeResult` cannot be made into an object
     |
     = note: method `from_ok` has no receiver
     = note: method `map_same` references the `Self` type in its arguments or return type

error[E0038]: the trait `ty::layout::MaybeResult` cannot be made into an object
    --> src/librustc/ty/layout.rs:1496:5
     |
1496 |     fn from(ty: TyLayout<'tcx>) -> dyn MaybeResult<TyLayout<'tcx>, Item=!> {
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `ty::layout::MaybeResult` cannot be made into an object
     |
     = note: method `from_ok` has no receiver
     = note: method `map_same` references the `Self` type in its arguments or return type

error: aborting due to 3 previous errors

Copy link
Contributor

Choose a reason for hiding this comment

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

oh... that was stupid of me. Sorry about that. It should suffice to add a : From<TyLayout<'tcx>> bound on the definition of MaybeResult.

This will end up requiring that all implementors of MaybeResult also implement From, not sure how well that works with the T impl

Copy link
Contributor

@saleemjaffer saleemjaffer Apr 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@oli-obk

I am trying something like this:

pub trait MaybeResult<'tcx, T>: From<TyLayout<'tcx>> {
    type Item;

    fn from_ok(x: T) -> Self;
    fn map_same<F: FnOnce(T) -> T>(self, f: F) -> Self;
    fn to_result(self) -> Result<T, Self::Item>;
}

impl<'tcx, T: From<TyLayout<'tcx>>> MaybeResult<'tcx, T> for T {
    type Item = !;

    fn from_ok(x: T) -> Self {
        x
    }
    fn map_same<F: FnOnce(T) -> T>(self, f: F) -> Self {
        f(self)
    }
    fn to_result(self) -> Result<T, !> {
        Ok(self)
    }
}

impl<'tcx, T, E> MaybeResult<'tcx, T> for Result<T, E> {
    type Item = E;

    fn from_ok(x: T) -> Self {
        Ok(x)
    }
    fn map_same<F: FnOnce(T) -> T>(self, f: F) -> Self {
        self.map(f)
    }
    fn to_result(self) -> Result<T, E> {
        self
    }
}

This is the fallout:

error[E0277]: the trait bound `std::result::Result<T, E>: std::convert::From<rustc_target::abi::TyLayout<'tcx, &'tcx ty::TyS<'tcx>>>` is not satisfied                      
    --> src/librustc/ty/layout.rs:1517:18
     |
1517 | impl<'tcx, T, E> MaybeResult<'tcx, T> for Result<T, E> {
     |                  ^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<rustc_target::abi::TyLayout<'tcx, &'tcx ty::TyS<'tcx>>>` is not implemented for `std::result::Result<T, E>`

error: aborting due to previous error

Should we implement From for Result? Does not seem right.

Copy link
Member

@eddyb eddyb Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for not giving a full signature, I think I meant this:

pub trait MaybeResult<T> {
    type Err;

    fn from(r: Result<T, Self::Err>) -> Self;
    fn to_result(self) -> Result<T, Self::Err>;
}

We can't use the From trait because T doesn't implement From<Result<T, !>> (nor does Result<T, !> implement Into<T>).

fn from_ok(x: T) -> Self;
fn map_same<F: FnOnce(T) -> T>(self, f: F) -> Self;
fn ok(self) -> Option<T>;
}

impl<T> MaybeResult<T> for T {
Expand All @@ -1506,6 +1509,9 @@ impl<T> MaybeResult<T> for T {
fn map_same<F: FnOnce(T) -> T>(self, f: F) -> Self {
f(self)
}
fn ok(self) -> Option<T> {
Some(self)
}
}

impl<T, E> MaybeResult<T> for Result<T, E> {
Expand All @@ -1515,6 +1521,9 @@ impl<T, E> MaybeResult<T> for Result<T, E> {
fn map_same<F: FnOnce(T) -> T>(self, f: F) -> Self {
self.map(f)
}
fn ok(self) -> Option<T> {
self.ok()
}
}

pub type TyLayout<'tcx> = ::rustc_target::abi::TyLayout<'tcx, Ty<'tcx>>;
Expand Down Expand Up @@ -1610,6 +1619,8 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
where C: LayoutOf<Ty = Ty<'tcx>> + HasTyCtxt<'tcx>,
C::TyLayout: MaybeResult<TyLayout<'tcx>>
{
type ParamEnv = ty::ParamEnv<'tcx>;

fn for_variant(this: TyLayout<'tcx>, cx: &C, variant_index: VariantIdx) -> TyLayout<'tcx> {
let details = match this.variants {
Variants::Single { index } if index == variant_index => this.details,
Expand Down Expand Up @@ -1762,6 +1773,131 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
}
})
}

fn pointee_info_at(
this: TyLayout<'tcx>,
cx: &C,
offset: Size,
param_env: Self::ParamEnv,
) -> Option<PointeeInfo> {
match this.ty.sty {
ty::RawPtr(mt) if offset.bytes() == 0 => {
cx.layout_of(mt.ty).ok()
.map(|layout| PointeeInfo {
size: layout.size,
align: layout.align.abi,
safe: None,
})
}

ty::Ref(_, ty, mt) if offset.bytes() == 0 => {
let tcx = cx.tcx();
let is_freeze = ty.is_freeze(tcx, param_env, DUMMY_SP);
let kind = match mt {
hir::MutImmutable => if is_freeze {
PointerKind::Frozen
} else {
PointerKind::Shared
},
hir::MutMutable => {
// Previously we would only emit noalias annotations for LLVM >= 6 or in
// panic=abort mode. That was deemed right, as prior versions had many bugs
// in conjunction with unwinding, but later versions didn’t seem to have
// said issues. See issue #31681.
//
// Alas, later on we encountered a case where noalias would generate wrong
// code altogether even with recent versions of LLVM in *safe* code with no
// unwinding involved. See #54462.
//
// For now, do not enable mutable_noalias by default at all, while the
// issue is being figured out.
let mutable_noalias = tcx.sess.opts.debugging_opts.mutable_noalias
.unwrap_or(false);
if mutable_noalias {
PointerKind::UniqueBorrowed
} else {
PointerKind::Shared
}
}
};

cx.layout_of(ty).ok()
.map(|layout| PointeeInfo {
size: layout.size,
align: layout.align.abi,
safe: Some(kind),
})
}

_ => {
let mut data_variant = match this.variants {
Variants::NicheFilling { dataful_variant, .. } => {
// Only the niche in this is always initialized,
// so only check for a pointer at its offset.
//
// If the niche is a pointer, it's either valid
// (according to its type), or null (which the
// niche field's scalar validity range encodes).
// This allows using `dereferenceable_or_null`
// for e.g., `Option<&T>`, and this will continue
// to work as long as we don't start using more
// niches than just null (e.g., the first page
// of the address space, or unaligned pointers).
if this.fields.offset(0) == offset {
Some(this.for_variant(cx, dataful_variant))
} else {
None
}
}
_ => Some(this)
};

if let Some(variant) = data_variant {
// We're not interested in any unions.
if let FieldPlacement::Union(_) = variant.fields {
data_variant = None;
}
}

let mut result = None;

if let Some(variant) = data_variant {
let ptr_end = offset + Pointer.size(cx);
for i in 0..variant.fields.count() {
let field_start = variant.fields.offset(i);
if field_start <= offset {
let field = variant.field(cx, i);
result = field.ok()
.and_then(|field| {
if ptr_end <= field_start + field.size {
let off = offset - field_start;
// We found the right field, look inside it.
Self::pointee_info_at(field, cx, off, param_env)
} else {
None
}
});
if result.is_some() {
break;
}
}
}
}

// FIXME(eddyb) This should be for `ptr::Unique<T>`, not `Box<T>`.
if let Some(ref mut pointee) = result {
if let ty::Adt(def, _) = this.ty.sty {
if def.is_box() && offset.bytes() == 0 {
pointee.safe = Some(PointerKind::UniqueOwned);
}
}
}

result
}
}
}

}

struct Niche {
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_codegen_llvm/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ use context::CodegenCx;
use rustc_codegen_ssa::mir::place::PlaceRef;
use rustc_codegen_ssa::mir::operand::OperandValue;
use type_::Type;
use type_of::{LayoutLlvmExt, PointerKind};
use type_of::{LayoutLlvmExt};
use value::Value;
use rustc_target::abi::call::ArgType;

use rustc_codegen_ssa::traits::*;

use rustc_target::abi::{HasDataLayout, LayoutOf, Size, TyLayout, Abi as LayoutAbi};
use rustc::ty::{self, Ty, Instance};
use rustc::ty::layout;
use rustc::ty::{self, Ty, Instance, ParamEnv};
use rustc::ty::layout::{self, PointerKind};

use libc::c_uint;

Expand Down Expand Up @@ -487,7 +487,7 @@ impl<'tcx> FnTypeExt<'tcx> for FnType<'tcx, Ty<'tcx>> {
}
}

if let Some(pointee) = layout.pointee_info_at(cx, offset) {
if let Some(pointee) = layout.pointee_info_at(cx, offset, ParamEnv::reveal_all()) {
if let Some(kind) = pointee.safe {
attrs.pointee_size = pointee.size;
attrs.pointee_align = Some(pointee.align);
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_codegen_llvm/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ use value::Value;

use monomorphize::partitioning::CodegenUnit;
use type_::Type;
use type_of::PointeeInfo;
use rustc_codegen_ssa::traits::*;
use libc::c_uint;

Expand All @@ -27,7 +26,7 @@ use rustc_data_structures::small_c_str::SmallCStr;
use rustc::mir::mono::Stats;
use rustc::session::config::{self, DebugInfo};
use rustc::session::Session;
use rustc::ty::layout::{LayoutError, LayoutOf, Size, TyLayout, VariantIdx};
use rustc::ty::layout::{LayoutError, LayoutOf, PointeeInfo, Size, TyLayout, VariantIdx};
use rustc::ty::{self, Ty, TyCtxt};
use rustc::util::nodemap::FxHashMap;
use rustc_target::spec::{HasTargetSpec, Target};
Expand Down
132 changes: 3 additions & 129 deletions src/librustc_codegen_llvm/type_of.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@

use abi::{FnType, FnTypeExt};
use common::*;
use rustc::hir;
use rustc::ty::{self, Ty, TypeFoldable};
use rustc::ty::layout::{self, Align, LayoutOf, Size, TyLayout};
use rustc_target::abi::FloatTy;
use rustc::ty::layout::{self, Align, LayoutOf, PointeeInfo, Size, TyLayout};
use rustc_target::abi::{FloatTy, TyLayoutMethods};
use rustc_mir::monomorphize::item::DefPathBasedNames;
use rustc_codegen_ssa::traits::*;
use type_::Type;
Expand Down Expand Up @@ -179,28 +178,6 @@ impl<'a, 'tcx> CodegenCx<'a, 'tcx> {
}
}

#[derive(Copy, Clone, PartialEq, Eq)]
pub enum PointerKind {
/// Most general case, we know no restrictions to tell LLVM.
Shared,

/// `&T` where `T` contains no `UnsafeCell`, is `noalias` and `readonly`.
Frozen,

/// `&mut T`, when we know `noalias` is safe for LLVM.
UniqueBorrowed,

/// `Box<T>`, unlike `UniqueBorrowed`, it also has `noalias` on returns.
UniqueOwned
}

#[derive(Copy, Clone)]
pub struct PointeeInfo {
pub size: Size,
pub align: Align,
pub safe: Option<PointerKind>,
}

pub trait LayoutLlvmExt<'tcx> {
fn is_llvm_immediate(&self) -> bool;
fn is_llvm_scalar_pair<'a>(&self) -> bool;
Expand Down Expand Up @@ -411,110 +388,7 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyLayout<'tcx> {
return pointee;
}

let mut result = None;
match self.ty.sty {
ty::RawPtr(mt) if offset.bytes() == 0 => {
let (size, align) = cx.size_and_align_of(mt.ty);
result = Some(PointeeInfo {
size,
align,
safe: None
});
}

ty::Ref(_, ty, mt) if offset.bytes() == 0 => {
let (size, align) = cx.size_and_align_of(ty);

let kind = match mt {
hir::MutImmutable => if cx.type_is_freeze(ty) {
PointerKind::Frozen
} else {
PointerKind::Shared
},
hir::MutMutable => {
// Previously we would only emit noalias annotations for LLVM >= 6 or in
// panic=abort mode. That was deemed right, as prior versions had many bugs
// in conjunction with unwinding, but later versions didn’t seem to have
// said issues. See issue #31681.
//
// Alas, later on we encountered a case where noalias would generate wrong
// code altogether even with recent versions of LLVM in *safe* code with no
// unwinding involved. See #54462.
//
// For now, do not enable mutable_noalias by default at all, while the
// issue is being figured out.
let mutable_noalias = cx.tcx.sess.opts.debugging_opts.mutable_noalias
.unwrap_or(false);
if mutable_noalias {
PointerKind::UniqueBorrowed
} else {
PointerKind::Shared
}
}
};

result = Some(PointeeInfo {
size,
align,
safe: Some(kind)
});
}

_ => {
let mut data_variant = match self.variants {
layout::Variants::NicheFilling { dataful_variant, .. } => {
// Only the niche itself is always initialized,
// so only check for a pointer at its offset.
//
// If the niche is a pointer, it's either valid
// (according to its type), or null (which the
// niche field's scalar validity range encodes).
// This allows using `dereferenceable_or_null`
// for e.g., `Option<&T>`, and this will continue
// to work as long as we don't start using more
// niches than just null (e.g., the first page
// of the address space, or unaligned pointers).
if self.fields.offset(0) == offset {
Some(self.for_variant(cx, dataful_variant))
} else {
None
}
}
_ => Some(*self)
};

if let Some(variant) = data_variant {
// We're not interested in any unions.
if let layout::FieldPlacement::Union(_) = variant.fields {
data_variant = None;
}
}

if let Some(variant) = data_variant {
let ptr_end = offset + layout::Pointer.size(cx);
for i in 0..variant.fields.count() {
let field_start = variant.fields.offset(i);
if field_start <= offset {
let field = variant.field(cx, i);
if ptr_end <= field_start + field.size {
// We found the right field, look inside it.
result = field.pointee_info_at(cx, offset - field_start);
break;
}
}
}
}

// FIXME(eddyb) This should be for `ptr::Unique<T>`, not `Box<T>`.
if let Some(ref mut pointee) = result {
if let ty::Adt(def, _) = self.ty.sty {
if def.is_box() && offset.bytes() == 0 {
pointee.safe = Some(PointerKind::UniqueOwned);
}
}
}
}
}
let result = Ty::pointee_info_at(*self, cx, offset, ty::ParamEnv::reveal_all());

cx.pointee_infos.borrow_mut().insert((self.ty, offset), result);
result
Expand Down
Loading