[WIP] Move pointee_info_at to TyLayoutMethods. #57150

wildarch · 2018-12-27T16:25:30Z

Resolves #56166.

This moves the pointee_info_at from rustc_codegen_llvm::type_of::LayoutLlvmExt to rustc::ty::layout::TyLayoutMethods.

This pull request is far from ready to be merged, and is only meant for an intermediate review. It will be expanded to a completely resolve #56166.

r? @eddyb

The original implementation is still present at librustc_codegen_llvm/abi.rs, should be removed later to prevent code duplication.

RalfJung · 2018-12-27T16:48:32Z

Uh I'm sorry don't think I can review this. I don't know this code.

r? @eddyb

oli-obk

Just a preliminary review.

Looks generally alright to me, can you start changing all the previous calls to pointee_info_at to this method and remove the old one?

src/librustc/ty/layout.rs

src/librustc_target/abi/mod.rs

Co-Authored-By: wildarch <[email protected]>

An associated type ParamEnv has been added to TyLayoutMethods to facilitate this.

rust-highfive · 2018-12-28T15:00:57Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:1547a3e4:start=1546007169673648626,finish=1546007225209864498,duration=55536215872
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:44] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:45] tidy error: /checkout/src/librustc/ty/layout.rs:1864: line longer than 100 chars
[00:03:46] some tidy checks failed
[00:03:46] 
[00:03:46] 
[00:03:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:46] 
[00:03:46] 
[00:03:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:46] Build completed unsuccessfully in 0:00:45
[00:03:46] Build completed unsuccessfully in 0:00:45
[00:03:46] make: *** [tidy] Error 1
[00:03:46] Makefile:69: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0c21d938
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Dec 28 14:31:00 UTC 2018
---
travis_time:end:095ec6a1:start=1546007461009059277,finish=1546007461015277412,duration=6218135
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00a0c460
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:01e3a6d6
travis_time:start:01e3a6d6
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0260a7c8
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

wildarch · 2019-01-11T10:34:20Z

@eddyb I think the moving of pointee_info_at is now complete. I've been looking at your hints for further refactoring in #56166, which part of rustc_codegen_ssa are you referring to there?

TimNN · 2019-01-29T18:12:53Z

Ping from triage @eddyb / @rust-lang/complier: This PR requires your review.

eddyb · 2019-01-31T09:02:10Z

src/librustc_target/abi/mod.rs

    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(


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

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?

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.

eddyb · 2019-01-31T09:11:23Z

My immediate reaction is that PointeeInfo is really about information LLVM wants, but I did suggest moving it, so it's probably fine (also, it's defined in broad Rust terms, not LLVM-specific distinctions).

@oli-obk Note that we never pass ParamEnv separately in layout code! It's always in the context, e.g.:

rust/src/librustc/ty/layout.rs

Lines 203 to 206 in f40aaa6

    
           pub struct LayoutCx<'tcx, C> { 
        
               pub tcx: C, 
        
               pub param_env: ty::ParamEnv<'tcx>, 
        
           }

or:

rust/src/librustc_mir/interpret/eval_context.rs

Lines 168 to 169 in f40aaa6

    
           fn layout_of(&self, ty: Ty<'tcx>) -> Self::TyLayout { 
        
               self.tcx.layout_of(self.param_env.and(ty))

So I think what is needed is to add a trait that contains is_freeze (or a way to get the ty::ParamEnv) as a bound on C here:

rust/src/librustc/ty/layout.rs

Lines 1599 to 1602 in f40aaa6

    
           impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx> 
        
               where C: LayoutOf<Ty = Ty<'tcx>> + HasTyCtxt<'tcx>, 
        
                     C::TyLayout: MaybeResult<TyLayout<'tcx>> 
        
           {

bors · 2019-02-07T09:52:36Z

☔ The latest upstream changes (presumably #58254) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC-zz · 2019-03-04T18:13:34Z

ping from triage @wildarch you need to reply to the review and make necessary changes. Also you have conflicts to attend to.

wildarch · 2019-03-07T09:53:19Z

@Dylan-DPC I have been a bit preoccupied as of late 😅, but I should have some time this weekend to attend to this. Thanks for the heads-up! 😄

wildarch · 2019-03-11T20:24:01Z

My immediate reaction is that PointeeInfo is really about information LLVM wants, but I did suggest moving it, so it's probably fine (also, it's defined in broad Rust terms, not LLVM-specific distinctions).

@oli-obk Note that we never pass ParamEnv separately in layout code! It's always in the context, e.g.:

rust/src/librustc/ty/layout.rs

Lines 203 to 206 in f40aaa6
pub struct LayoutCx<'tcx, C> {
pub tcx: C,
pub param_env: ty::ParamEnv<'tcx>,
}

or:

rust/src/librustc_mir/interpret/eval_context.rs

Lines 168 to 169 in f40aaa6
fn layout_of(&self, ty: Ty<'tcx>) -> Self::TyLayout {
self.tcx.layout_of(self.param_env.and(ty))

So I think what is needed is to add a trait that contains is_freeze (or a way to get the ty::ParamEnv) as a bound on C here:

rust/src/librustc/ty/layout.rs

Lines 1599 to 1602 in f40aaa6
impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
where C: LayoutOf<Ty = Ty<'tcx>> + HasTyCtxt<'tcx>,
C::TyLayout: MaybeResult<TyLayout<'tcx>>
{

I feel like a trait just for is_freeze might be a little too specific, I think it would be nice to have a trait like:

pub trait HasParamEnv {
    fn param_env(&self) -> ParamEnv
}

And then put that as an additional bound on C, which I think is what you suggested with ty::ParamEnv?

Then I think I should be able to remove the ParamEnv as a parameter from the pointee_info_at function and use the available context to retrieve it. Does that sound alright to you @eddyb?

eddyb · 2019-03-13T10:25:29Z

src/librustc/ty/layout.rs

@@ -1497,6 +1499,7 @@ impl<'gcx, 'tcx, T: HasTyCtxt<'gcx>> HasTyCtxt<'gcx> for LayoutCx<'tcx, T> {
 pub trait MaybeResult<T> {


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()

@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.

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.

@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

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

@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.

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>).

eddyb · 2019-03-13T10:26:14Z

@wildarch Sure, either way should work fine. Note that is_freeze could be added to TyLayoutMethods, which might be the nicest way to make this work.

Centril · 2019-03-30T12:36:26Z

Ping from triage @wildarch, any progress?

wildarch · 2019-04-02T21:36:36Z

@Centril I have been really busy lately, so I have not had much time to look at this. I will try to find more time to continue the PR, but honestly it might be better if someone else would take over the responsibility of finishing this. Sorry to keep you waiting 😢

saleemjaffer · 2019-04-24T09:08:25Z

@oli-obk @eddyb I can take this up!

@oli-obk

…ng, r=eddyb Move pointee_info_at from rustc_codegen_llvm to rustc_target. Makes progress towards #56166. This is a continuation of #57150. @oli-obk Should I close the older PR?

Centril · 2019-05-18T10:47:06Z

I believe #60237 implemented this, so we can close the PR. Please reopen if I am wrong.

Move pointee_info_at to TyLayoutMethods.

15955b9

The original implementation is still present at librustc_codegen_llvm/abi.rs, should be removed later to prevent code duplication.

rust-highfive assigned RalfJung Dec 27, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2018

rust-highfive assigned eddyb and unassigned RalfJung Dec 27, 2018

oli-obk requested changes Dec 28, 2018

View reviewed changes

oli-obk and others added 4 commits December 28, 2018 12:23

Fix typo in src/librustc/ty/layout.rs

659ffb2

Co-Authored-By: wildarch <[email protected]>

Return instead of collecting to mut result.

3d3d64f

Remove old pointee_info_at body.

e08d9cf

Add param_env parameter to pointee_info_at.

7cba55e

An associated type ParamEnv has been added to TyLayoutMethods to facilitate this.

Make line fit within 100 character limit.

bb05cb8

eddyb reviewed Jan 31, 2019

View reviewed changes

impl pointee_info_at in TyLayout.

3ce7a68

eddyb reviewed Mar 13, 2019

View reviewed changes

TimNN added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2019

saleemjaffer mentioned this pull request Apr 24, 2019

Move pointee_info_at from rustc_codegen_llvm to rustc_target. #60237

Merged

Centril closed this May 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Move pointee_info_at to TyLayoutMethods. #57150

[WIP] Move pointee_info_at to TyLayoutMethods. #57150

wildarch commented Dec 27, 2018 •

edited

Loading

RalfJung commented Dec 27, 2018

oli-obk left a comment

rust-highfive commented Dec 28, 2018

wildarch commented Jan 11, 2019

TimNN commented Jan 29, 2019

eddyb Jan 31, 2019

wildarch Mar 11, 2019

eddyb Mar 13, 2019

eddyb commented Jan 31, 2019

bors commented Feb 7, 2019

Dylan-DPC-zz commented Mar 4, 2019

wildarch commented Mar 7, 2019

wildarch commented Mar 11, 2019

eddyb Mar 13, 2019

saleemjaffer Apr 25, 2019 •

edited

Loading

oli-obk Apr 25, 2019

saleemjaffer Apr 26, 2019

oli-obk Apr 28, 2019

saleemjaffer Apr 28, 2019 •

edited

Loading

eddyb Apr 30, 2019 •

edited

Loading

eddyb commented Mar 13, 2019

Centril commented Mar 30, 2019

wildarch commented Apr 2, 2019

saleemjaffer commented Apr 24, 2019

Centril commented May 18, 2019

		@@ -1497,6 +1499,7 @@ impl<'gcx, 'tcx, T: HasTyCtxt<'gcx>> HasTyCtxt<'gcx> for LayoutCx<'tcx, T> {
		pub trait MaybeResult<T> {

[WIP] Move pointee_info_at to TyLayoutMethods. #57150

[WIP] Move pointee_info_at to TyLayoutMethods. #57150

Conversation

wildarch commented Dec 27, 2018 • edited Loading

RalfJung commented Dec 27, 2018

oli-obk left a comment

Choose a reason for hiding this comment

rust-highfive commented Dec 28, 2018

wildarch commented Jan 11, 2019

TimNN commented Jan 29, 2019

eddyb Jan 31, 2019

Choose a reason for hiding this comment

wildarch Mar 11, 2019

Choose a reason for hiding this comment

eddyb Mar 13, 2019

Choose a reason for hiding this comment

eddyb commented Jan 31, 2019

bors commented Feb 7, 2019

Dylan-DPC-zz commented Mar 4, 2019

wildarch commented Mar 7, 2019

wildarch commented Mar 11, 2019

eddyb Mar 13, 2019

Choose a reason for hiding this comment

saleemjaffer Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

oli-obk Apr 25, 2019

Choose a reason for hiding this comment

saleemjaffer Apr 26, 2019

Choose a reason for hiding this comment

oli-obk Apr 28, 2019

Choose a reason for hiding this comment

saleemjaffer Apr 28, 2019 • edited Loading

Choose a reason for hiding this comment

eddyb Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

eddyb commented Mar 13, 2019

Centril commented Mar 30, 2019

wildarch commented Apr 2, 2019

saleemjaffer commented Apr 24, 2019

Centril commented May 18, 2019

wildarch commented Dec 27, 2018 •

edited

Loading

saleemjaffer Apr 25, 2019 •

edited

Loading

saleemjaffer Apr 28, 2019 •

edited

Loading

eddyb Apr 30, 2019 •

edited

Loading