Skip to content

/rust

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

trans: always use a memcpy for ABI argument/return casts. #34141

Merged
merged 1 commit into from Jun 8, 2016

Conversation

Reviewers
No reviews
Assignees

@nrc nrc

Labels
T-compiler beta-accepted
Projects
None yet
Milestone
No milestone
9 participants
@eddyb @rust-highfive @nikomatsakis @bors @nagisa @dotdash @arielb1 @brson @nrc
@eddyb
Copy link
Member

eddyb commented Jun 7, 2016

When storing incoming arguments or values returned by call/invoke, always do a memcpy from a temporary of the cast type, if there is an ABI cast.
While Clang has gotten smarter (store vs memcpy), a memcpy will always work.
This is what @dotdash has wanted to do all along, and it fixes #32049.

@rust-highfive rust-highfive assigned nrc Jun 7, 2016

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jun 7, 2016

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb eddyb added the beta-nominated label Jun 7, 2016

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member Author

eddyb commented Jun 7, 2016

Nominated for backporting to beta because it reverts #33872's effect of triggering #32049 in the image crate. #33872 was already accepted, and this PR can be seen as a "more correct" alternative to that.
src/librustc_trans/abi.rs Outdated

// We instead thus allocate some scratch space...
let llscratch = AllocaFcx(bcx.fcx(), ty, "abi_cast");
base::Lifetime::End.call(bcx, llscratch);

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Jun 7, 2016

Contributor

shouldn't this be lifetime start?

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Jun 7, 2016

Author Member

Oops...

@eddyb eddyb force-pushed the eddyb:trans-abi-memcpy branch from d43abf6 to 2e68f31 Jun 7, 2016

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jun 7, 2016

@bors r+ p=1
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit 2e68f31 has been approved by nikomatsakis
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jun 7, 2016

I'm inclined to beta-accept -- thoughts from @rust-lang/compiler team?
@nagisa

This comment has been minimized.

Sign in to view
Copy link
Contributor

nagisa commented Jun 7, 2016 •
edited

I’m fine with it. My fix wasn’t supposed to be more than a short-term quick-hack anyway (for certain time-pressing reasons like trains rolling over the next day).

EDIT: or rather, please do accept it.

@eddyb eddyb force-pushed the eddyb:trans-abi-memcpy branch from 2e68f31 to 0d145c9 Jun 7, 2016

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member Author

eddyb commented Jun 7, 2016

@bors r=nikomatsakis p=2

I've rebased this on top of #34128 because it's likely to get in and because I won't be around to babysit it.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit 0d145c9 has been approved by nikomatsakis
@dotdash

This comment has been minimized.

Sign in to view
Copy link
Contributor

dotdash commented Jun 7, 2016

+1 on the backport
src/librustc_trans/base.rs Outdated
})
impl Lifetime {
pub fn call(self, b: &Builder, ptr: ValueRef) {
core_lifetime_emit(b.ccx, ptr, Lifetime::Start, |ccx, size, lifetime_intrinsic| {

This comment has been minimized.

Sign in to view
Copy link
@dotdash

dotdash Jun 7, 2016

Contributor

I think this needs to be self instead of Lifetime::Start?

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Jun 7, 2016

Author Member

I... wrote this twice. It was fine the first time :/.
@dotdash

This comment has been minimized.

Sign in to view
Copy link
Contributor

dotdash commented Jun 7, 2016

Yep, that's much closer (not sure if 100% the same) to what I had in mind. Sorry for not getting that done myself earlier and causing some unnecessary churn :-/

That should also be useful for the outstanding transmute fix of mine that's still missing its MIR part.

Nice!

@eddyb eddyb force-pushed the eddyb:trans-abi-memcpy branch from 0d145c9 to d9f93b9 Jun 7, 2016

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member Author

eddyb commented Jun 7, 2016

@bors r=nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit d9f93b9 has been approved by nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

⌛️ Testing commit d9f93b9 with merge ab5b316...

bors added a commit that referenced this pull request Jun 7, 2016

@bors
Auto merge of #34141 - eddyb:trans-abi-memcpy, r=nikomatsakis 
trans: always use a memcpy for ABI argument/return casts.

When storing incoming arguments or values returned by call/invoke, always do a `memcpy` from a temporary of the cast type, if there is an ABI cast.
While Clang has gotten smarter ([store](https://godbolt.org/g/EphFuK) vs [memcpy](https://godbolt.org/g/5dikH9)), a `memcpy` will always work.
This is what @dotdash has wanted to do all along, and it fixes #32049.
ab5b316
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild
@arielb1

This comment has been minimized.

Sign in to view
Copy link
Contributor

arielb1 commented Jun 7, 2016 •
edited

Real failure:

C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src/test\codegen\stores.rs:34:11: error: expected string not found in input
// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[Y8]], i8* [[TMP8]], i64 4, i32 1, i1 false)
          ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\i686-pc-windows-gnu\test\codegen\stores.ll:19:2: note: scanning from here
 call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %4, i32 4, i32 1, i1 false)
 ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\i686-pc-windows-gnu\test\codegen\stores.ll:19:2: note: with variable "Y8" equal to "%3"
 call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %4, i32 4, i32 1, i1 false)
 ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\i686-pc-windows-gnu\test\codegen\stores.ll:19:2: note: with variable "TMP8" equal to "%4"
 call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %4, i32 4, i32 1, i1 false)
 ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src/test\codegen\stores.rs:49:11: error: expected string not found in input
// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* [[Y8]], i8* [[TMP8]], i64 4, i32 1, i1 false)
          ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\i686-pc-windows-gnu\test\codegen\stores.ll:50:2: note: scanning from here
 call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %4, i32 4, i32 1, i1 false)
 ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\i686-pc-windows-gnu\test\codegen\stores.ll:50:2: note: with variable "Y8" equal to "%3"
 call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %4, i32 4, i32 1, i1 false)
 ^
C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\i686-pc-windows-gnu\test\codegen\stores.ll:50:2: note: with variable "TMP8" equal to "%4"
 call void @llvm.memcpy.p0i8.p0i8.i32(i8* %3, i8* %4, i32 4, i32 1, i1 false)

Difference: @llvm.memcpy.p0i8.p0i8.i64 vs. @llvm.memcpy.p0i8.p0i8.i32
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member Author

eddyb commented Jun 7, 2016

@arielb1 Who needs 32-bit anyway /s.

@eddyb eddyb force-pushed the eddyb:trans-abi-memcpy branch from d9f93b9 to e252865 Jun 7, 2016

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member Author

eddyb commented Jun 7, 2016

@bors r=nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

📌 Commit e252865 has been approved by nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 7, 2016

⌛️ Testing commit e252865 with merge ec872dc...

bors added a commit that referenced this pull request Jun 7, 2016

@bors
Auto merge of #34141 - eddyb:trans-abi-memcpy, r=nikomatsakis 
trans: always use a memcpy for ABI argument/return casts.

When storing incoming arguments or values returned by call/invoke, always do a `memcpy` from a temporary of the cast type, if there is an ABI cast.
While Clang has gotten smarter ([store](https://godbolt.org/g/EphFuK) vs [memcpy](https://godbolt.org/g/5dikH9)), a `memcpy` will always work.
This is what @dotdash has wanted to do all along, and it fixes #32049.
ec872dc
@bors

@bors bors merged commit e252865 into rust-lang:master Jun 8, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details

@eddyb eddyb deleted the eddyb:trans-abi-memcpy branch Jun 8, 2016

@arielb1 arielb1 added the T-compiler label Jun 9, 2016

@nikomatsakis nikomatsakis added the beta-accepted label Jun 9, 2016

@MagaTailor MagaTailor referenced this pull request Jun 10, 2016

Closed

LTO segfaults in llvm #32272

@alexcrichton alexcrichton referenced this pull request Jun 28, 2016

Merged

Backport #34141 to beta #34533

@brson brson removed the beta-nominated label Jun 28, 2016

brson added a commit that referenced this pull request Jun 28, 2016

@brson
Merge pull request #34533 from alexcrichton/beta-next 
Backport #34141 to beta
77c9c08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.