Improve raw Box conversions #44877

nvzqz · 2017-09-27T01:22:20Z

This PR has two goals:

Reduce use of mem::transmute in Box conversions

I understand that mem::transmute-ing non #[repr(C)] types is implementation-defined behavior. This may not matter within the reference implementation of Rust, but I believe it's important to remain consistent. For example, I noticed that str::from_utf8_unchecked went from using mem::transmute to using pointer casts.
Make Box pointer conversions more straightforward regarding Unique

Makes use of conversions via Unique.

nvzqz · 2017-09-27T13:37:54Z

This change caused this error:

[00:03:49] error: internal compiler error: /checkout/src/librustc_trans/adt.rs:431: Cannot handle boxed::Box<[u8]> represented as TyLayout {
[00:03:49]     ty: boxed::Box<[u8]>,
[00:03:49]     layout: FatPointer {
[00:03:49]         metadata: Int(
[00:03:49]             I64
[00:03:49]         ),
[00:03:49]         non_zero: true
[00:03:49]     },
[00:03:49]     variant_index: None
[00:03:49] }

When running

stamp sh -x -c "$RUN_SCRIPT"

I'm not sure how to go about fixing this.

nvzqz · 2017-09-28T00:29:20Z

Would you mind taking a look at this @alexcrichton?

aidanhs · 2017-09-28T00:46:52Z

Thanks for the PR @nvzqz! We'll check in now and again to make sure @alexcrichton or another reviewer gets to this soon.

alexcrichton · 2017-09-28T02:03:50Z

Thanks for the PR @nvzqz! As of a very long time ago (maybe this is still true today?) the Box type was defined in the compiler and the type definition here in the libraries mainly only sufficed for a few analysis passes in rustc. I think that may be where the error is coming from here? Maybe the tyep is still defined in the compiler and as a result trying to interpret it is causing issues?

nagisa · 2017-09-28T17:51:38Z

src/liballoc/boxed.rs

@@ -326,7 +326,9 @@ impl<T: ?Sized> Box<T> {
               issue = "27730")]
    #[inline]
    pub fn into_unique(b: Box<T>) -> Unique<T> {
-        unsafe { mem::transmute(b) }
+        let u = b.0;


The error is most likely arising due to this line in particular.

If it is indeed the culprit, it is pretty easy to see why is this a case -- namely within the compiler it is considered that Box is pretty much equivalent to a non-aliasable *mut T, and you cannot just take fields of *mut Ts without deferencing first -- hence the ICE you see.

If you want, you could try fixing it in the compiler, but I’m fairly sure it would be very messy to do so properly.

Disclaimer: This is a guess.

Remove mem::transmute used in Box<str> conversions Given that #44877 is failing, I decided to make a separate PR. This is done with the same motivation: to avoid `mem::transmute`-ing non `#[repr(C)]` types.

shepmaster · 2017-10-06T18:26:50Z

Heya, @nvzqz, it's been over 7 days since you last checked in; are you still working on this?

Seems to cause this error: "Cannot handle boxed::Box<[u8]> represented as TyLayout".

nvzqz · 2017-10-06T20:42:05Z

@shepmaster just pushed another commit that'll hopefully make this work by reverting one of the changes. I don't know nearly enough about the compiler internals to fix the root issue.

Same reasons as commit 904133e.

nvzqz · 2017-10-06T21:09:41Z

I'm interested in adding Box::from_unique and making it unstable under #27730.

Provides a reasonable interface for Box::from_raw implementation. Does not get around the requirement of mem::transmute for converting back and forth between Unique and Box.

alexcrichton · 2017-10-09T21:23:10Z

@bors: r+

Thanks!

bors · 2017-10-09T21:23:10Z

📌 Commit 22298b8 has been approved by alexcrichton

bors · 2017-10-10T11:07:30Z

⌛ Testing commit 22298b8 with merge 0217315...

Improve raw Box conversions This PR has two goals: - Reduce use of `mem::transmute` in `Box` conversions I understand that `mem::transmute`-ing non `#[repr(C)]` types is implementation-defined behavior. This may not matter within the reference implementation of Rust, but I believe it's important to remain consistent. For example, I noticed that `str::from_utf8_unchecked` went from using `mem::transmute` to using pointer casts. - Make `Box` pointer conversions more straightforward regarding `Unique`

bors · 2017-10-10T13:34:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0217315 to master...

nvzqz added 2 commits September 26, 2017 21:16

Remove uses of mem::transmute in Box methods

edb2c2d

Makes use of conversions via Unique.

Merge remote-tracking branch 'upstream/master' into box-conversions

8f167b3

nvzqz changed the title ~~Remove use of mem::transmute in some Box conversions~~ Remove use of mem::transmute in raw Box conversions Sep 27, 2017

nvzqz mentioned this pull request Sep 27, 2017

Remove mem::transmute used in Box<str> conversions #44890

Merged

aidanhs assigned alexcrichton Sep 28, 2017

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2017

nagisa reviewed Sep 28, 2017

View reviewed changes

carols10cents 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 Oct 2, 2017

Revert to using mem::transmute in Box::into_unique

904133e

Seems to cause this error: "Cannot handle boxed::Box<[u8]> represented as TyLayout".

Revert to using mem::transmute in Box::from_raw

452b71a

Same reasons as commit 904133e.

Create Box::from_unique function

5ce5b2f

Provides a reasonable interface for Box::from_raw implementation. Does not get around the requirement of mem::transmute for converting back and forth between Unique and Box.

nvzqz changed the title ~~Remove use of mem::transmute in raw Box conversions~~ Improve raw Box conversions Oct 6, 2017

nvzqz added 2 commits October 6, 2017 17:39

Add missing word in Box::from_unique docs

5af88ee

Add unique feature in Box::from_unique docs

22298b8

bors merged commit 22298b8 into rust-lang:master Oct 10, 2017

nvzqz deleted the box-conversions branch December 25, 2017 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve raw Box conversions #44877

Improve raw Box conversions #44877

nvzqz commented Sep 27, 2017 •

edited

Loading

nvzqz commented Sep 27, 2017

nvzqz commented Sep 28, 2017

aidanhs commented Sep 28, 2017

alexcrichton commented Sep 28, 2017

nagisa Sep 28, 2017 •

edited

Loading

shepmaster commented Oct 6, 2017

nvzqz commented Oct 6, 2017

nvzqz commented Oct 6, 2017 •

edited

Loading

alexcrichton commented Oct 9, 2017

bors commented Oct 9, 2017

bors commented Oct 10, 2017

bors commented Oct 10, 2017

Improve raw Box conversions #44877

Improve raw Box conversions #44877

Conversation

nvzqz commented Sep 27, 2017 • edited Loading

nvzqz commented Sep 27, 2017

nvzqz commented Sep 28, 2017

aidanhs commented Sep 28, 2017

alexcrichton commented Sep 28, 2017

nagisa Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

shepmaster commented Oct 6, 2017

nvzqz commented Oct 6, 2017

nvzqz commented Oct 6, 2017 • edited Loading

alexcrichton commented Oct 9, 2017

bors commented Oct 9, 2017

bors commented Oct 10, 2017

bors commented Oct 10, 2017

nvzqz commented Sep 27, 2017 •

edited

Loading

nagisa Sep 28, 2017 •

edited

Loading

nvzqz commented Oct 6, 2017 •

edited

Loading