Skip to content

Fix handling of FFI arguments #33872

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
Merged
merged 6 commits into from
May 26, 2016
Merged

Fix handling of FFI arguments #33872

merged 6 commits into from
May 26, 2016

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented May 25, 2016

r? @eddyb @nikomatsakis or whoever else.

cc @alexcrichton @rust-lang/core

The strategy employed here was to essentially change code we generate from

  %s = alloca %S ; potentially smaller than argument, but never larger
  %1 = bitcast %S* %s to { i64, i64 }*
  store { i64, i64 } %0, { i64, i64 }* %1, align 4

to

  %1 = alloca { i64, i64 } ; the copy of argument itself
  store { i64, i64 } %0, { i64, i64 }* %1, align 4
  %s = bitcast { i64, i64 }* %1 to %S* ; potentially truncate by casting to a pointer of smaller type.

@nagisa nagisa added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 25, 2016
@nagisa
Copy link
Member Author

nagisa commented May 25, 2016

Hmm, it clashes with debuginfo.

eddyb
eddyb reviewed May 25, 2016
View reviewed changes
src/librustc_trans/base.rs

// FIXME: hacky lol?
datum::Datum::new(lltmp, arg_ty,
datum::Lvalue::new("datum::lvalue_scratch_datum"))
Copy link
Member

Choose a reason for hiding this comment

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

This won't work with debuginfo, which wants an alloca with no casts on top.
I would suggest giving it the alloca before the cast and hope that it doesn't care about the type (debuginfo metadata should describe the actual value in the alloca all by itself).
cc @michaelwoerister

@alexcrichton
Copy link
Member

Thanks for the quick fix @nagisa!

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 26, 2016
@nagisa nagisa force-pushed the undef-is-llvm-for-sigsegv branch from ae8fc5c to 0d2a84c Compare May 26, 2016 00:26
@nagisa
Copy link
Member Author

nagisa commented May 26, 2016

Added a check for debuginfo thing. All should work now.

If there’s any hard-pressing nits, feel to fork and make another PR as I’m likely to be in bed for a while now.

@eddyb
Copy link
Member

eddyb commented May 26, 2016

@bors r+ p=100

@bors
Copy link
Collaborator

bors commented May 26, 2016

📌 Commit e0e50a4 has been approved by eddyb

@bors
Copy link
Collaborator

bors commented May 26, 2016

⌛ Testing commit e0e50a4 with merge 73fdf78...

@bors
Copy link
Collaborator

bors commented May 26, 2016

💔 Test failed - auto-linux-64-debug-opt

@eddyb
Copy link
Member

eddyb commented May 26, 2016

Miscompiling the compiler, resulting in this for stage2 libcore:

thread 'rustc' panicked at 'RefCell<T> already borrowed', ../src/libcore/cell.rs:492

@alexcrichton Serious question: why is RUST_BACKTRACE=1 not set on the bots? I assume it has to do with some tests not taking that very well?

@alexcrichton
Copy link
Member

No particular reason, it just hasn't been done yet. We'd probably benefit from just setting it in the build system directly.

@nagisa
Copy link
Member Author

nagisa commented May 26, 2016

@bors r=eddyb 2f0da79

@nagisa
Copy link
Member Author

nagisa commented May 26, 2016

@bors r=eddyb 5b40452

@bors
Copy link
Collaborator

bors commented May 26, 2016

⌛ Testing commit 5b40452 with merge 3c795e0...

bors added a commit that referenced this pull request May 26, 2016
@bors
Auto merge of #33872 - nagisa:undef-is-llvm-for-sigsegv, r=eddyb
3c795e0 
Fix handling of FFI arguments

r? @eddyb @nikomatsakis or whoever else.

cc @alexcrichton @rust-lang/core

The strategy employed here was to essentially change code we generate from

```llvm
  %s = alloca %S ; potentially smaller than argument, but never larger
  %1 = bitcast %S* %s to { i64, i64 }*
  store { i64, i64 } %0, { i64, i64 }* %1, align 4
```

to

```llvm
  %1 = alloca { i64, i64 } ; the copy of argument itself
  store { i64, i64 } %0, { i64, i64 }* %1, align 4
  %s = bitcast { i64, i64 }* %1 to %S* ; potentially truncate by casting to a pointer of smaller type.
```
@bors
Copy link
Collaborator

bors commented May 26, 2016

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

@nagisa
Copy link
Member Author

nagisa commented May 26, 2016

@bors retry force

@bors
Copy link
Collaborator

bors commented May 26, 2016

⚡ Previous build results for auto-linux-64-cargotest, auto-linux-cross-opt, auto-win-msvc-64-cargotest are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild...

@sanxiyn
Copy link
Member

sanxiyn commented May 26, 2016

Another case of #33844.

@bors
Copy link
Collaborator

bors commented May 26, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 5b40452 into rust-lang:master May 26, 2016
@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 26, 2016
@alexcrichton
Copy link
Member

Discussed on IRC and accepted for a beta backport

@alexcrichton alexcrichton mentioned this pull request May 26, 2016
Merged
This was referenced Jun 7, 2016
Closed
Merged
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nagisa @alexcrichton @eddyb @bors @sanxiyn