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

[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 1 commit
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
128 changes: 128 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 @@ -1762,6 +1771,125 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
}
})
}

fn pointee_info_at(this: TyLayout<'tcx>, cx: &C, offset: Size
wildarch marked this conversation as resolved.
Show resolved Hide resolved
) -> Option<PointeeInfo> {
let mut result = None;
wildarch marked this conversation as resolved.
Show resolved Hide resolved
match this.ty.sty {
ty::RawPtr(mt) if offset.bytes() == 0 => {
result = 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, ty::ParamEnv::reveal_all(), DUMMY_SP);
wildarch marked this conversation as resolved.
Show resolved Hide resolved
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
}
}
};

result = 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 itthis is always initialized,
wildarch marked this conversation as resolved.
Show resolved Hide resolved
// 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;
}
}

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 {
// We found the right field, look inside it.
Self::pointee_info_at(field, cx, offset - field_start)
} 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
27 changes: 27 additions & 0 deletions src/librustc_target/abi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,13 +913,40 @@ pub trait LayoutOf {
fn layout_of(&self, ty: Self::Ty) -> Self::TyLayout;
}

#[derive(Copy, Clone, PartialEq, Eq)]
pub enum PointerKind {
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
/// 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 TyLayoutMethods<'a, C: LayoutOf<Ty = Self>>: Sized {
fn for_variant(
this: TyLayout<'a, Self>,
cx: &C,
variant_index: VariantIdx,
) -> TyLayout<'a, Self>;
fn field(this: TyLayout<'a, Self>, cx: &C, i: usize) -> C::TyLayout;
fn pointee_info_at(
Copy link
Member

Choose a reason for hiding this comment

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

Each method in this trait needs a "forwarding" wrapper in the impl just below, the trait should never be used otherwise, only implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that should be easy to add 👍. As far as I could tell I am never actually using the trait directly, so that should be alright. Maybe you could give me some more details on the design decision here, so that I can leave it as a comment for the TyLayoutMethods trait?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. We have to implement the trait on the Ty (e.g. rustc::ty::Ty<'tcx>), but the functionality should be available as method syntax on TyLayout<Ty>.

If we make TyLayout<Ty> Deref to Ty, we can use self instead of this and have the trait directly callable.
But it already Derefs to LayoutDetails, sadly.

this: TyLayout<'a, Self>,
cx: &C,
offset: Size
) -> Option<PointeeInfo>;
}

impl<'a, Ty> TyLayout<'a, Ty> {
Expand Down