Fix ICE: inject bitcast if types mismatch for invokes/calls/stores by pnkfelix · Pull Request #37112 · rust-lang/rust

pnkfelix · 2016-10-12T15:02:01Z

Fix ICE: inject bitcast if types mismatch for invokes/calls

Partial fix for #36744

rust-highfive · 2016-10-12T15:02:15Z

r? @Aatch

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

pnkfelix · 2016-10-12T15:02:47Z

(oh I forgot a regression test; will put into followup commit.)

pnkfelix · 2016-10-12T15:32:16Z

So I just realized that this fix as originally written only handles the case of calls, which does fix the specific report of #36744 but does not address the other cases that @arielb1 pointed out here: #36744 (comment)

I'm looking into generalizing it slightly now to deal with more of those.

Ideally we would handle the following cases (I will check off the ones that the PR handles as I go):

call arguments (when parameters have non-aggregate type)
stores (of any kind)
call arguments of aggregate type (cannot use naive bitcast...)
- hmm, I have not yet managed to create an test case that exposes the issue I worry about here.
- niko notes that we don't pass tuples directly in calls; we instead allocate the tuple locally and pass a pointer to it.
- ariel notes that stores can happen via intrinsics or elsewhere when one has RVO.
call arguments of aggregate type (#[repr(C)] struct) to an extern "C" fn
- this is an exception to above claim that aggregates are handled via passing pointers, and will not be handled properly in bitcasts
return instruction
- It looks like this case is covered by the code I added for building store instructions

I'm also considering taking a different tack of extending MIR with an explicit upcast operator so that trans can remain simple. But I would be less inclined to back port a PR that takes that route.

… or calls.

pnkfelix · 2016-10-12T16:32:39Z

While working on this PR, I realized that one flaw in this strategy is that you can not apply the LLVM bitcast instruction to aggregates. This may mean that we have other cases to handle that simply cannot be dealt with via the means used here.

pnkfelix · 2016-10-13T12:56:26Z

r? @nikomatsakis

pnkfelix · 2016-10-13T21:52:57Z

FYI @arielb1 mentioned over IRC discussion that he would prefer a more targeted fix oriented around changes to MIR-trans.

pnkfelix · 2016-10-17T15:27:20Z

@nikomatsakis @arielb1 :

We should decide whether we are going to land this patch or not.

my opinion is that we should put this PR in. It solves the bulk of the occurrences of this problem (see above comment), and guards against adding the bitcast when it is unneeded by first checking if the types already match.

arielb1 · 2016-10-17T16:25:17Z

Sure. My patch turned out to be more complicated than it seems.

@bors r+

bors · 2016-10-17T16:25:20Z

📌 Commit 0562654 has been approved by arielb1

bors · 2016-10-17T16:26:14Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Lint against lowercase static mut #37162

bors · 2016-10-17T16:26:14Z

📌 Commit 0562654 has been approved by arielb1

Fix ICE: inject bitcast if types mismatch for invokes/calls/stores Fix ICE: inject bitcast if types mismatch for invokes/calls Fix#36744

bors · 2016-10-17T16:47:47Z

⌛ Testing commit 0562654 with merge ce31626...

nikomatsakis

r=me modulo nit

nikomatsakis · 2016-10-17T17:53:07Z


-fn check_call(typ: &str, llfn: ValueRef, args: &[ValueRef]) {
- if cfg!(debug_assertions) {
+ fn check_call<'b>(&self,


I think a comment here would be nice-- what is this Cow that is being returned?

nikomatsakis · 2016-10-17T17:55:13Z

(Oh, I see @arielb1 already r+'d. Seems fine.)

bors · 2016-10-17T21:06:44Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, 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-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: arielb1
Pushing ce31626 to master...

nikomatsakis · 2016-10-18T17:42:55Z

Accepting for beta. Low risk patch, regression.

cc @rust-lang/compiler

rust-highfive assigned Aatch Oct 12, 2016

Fix ICE by injecting bitcasts if types mismatch when building invokes…
97e4143
… or calls.

pnkfelix added 2 commits October 12, 2016 19:26

Inject bitcast if types mismatch when building a store instruction.
f166930

Some tests to check that lifetime parametric fn's do not trip up LLVM.
0562654

rust-highfive assigned nikomatsakis and unassigned Aatch Oct 13, 2016

pnkfelix changed the title ~~Fix ICE: inject bitcast if types mismatch for invokes/calls~~Fix ICE: inject bitcast if types mismatch for invokes/calls/storesOct 13, 2016

brson mentioned this pull request Oct 14, 2016
[stable] Backports for 1.12.1 #37173
Merged

bors added a commit that referenced this pull request Oct 17, 2016
Auto merge of#37112- pnkfelix:fix-issue-36744, r=arielb1
ce31626
Fix ICE: inject bitcast if types mismatch for invokes/calls/stores Fix ICE: inject bitcast if types mismatch for invokes/calls Fix#36744

nikomatsakis approved these changes Oct 17, 2016
View reviewed changes

bors merged commit 0562654 into rust-lang:masterOct 17, 2016

brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 18, 2016

brson mentioned this pull request Oct 18, 2016
[beta] backports #37239
Merged

nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 18, 2016

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 18, 2016

nikomatsakis mentioned this pull request Oct 18, 2016
Backport fix for 36744 to stable #37260
Closed

Uh oh!

Uh oh!

Conversation

pnkfelix commented Oct 12, 2016• edited by arielb1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Oct 12, 2016

Uh oh!

pnkfelix commented Oct 12, 2016

Uh oh!

pnkfelix commented Oct 12, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Oct 12, 2016

Uh oh!

pnkfelix commented Oct 13, 2016

Uh oh!

pnkfelix commented Oct 13, 2016

Uh oh!

pnkfelix commented Oct 17, 2016

Uh oh!

arielb1 commented Oct 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Oct 17, 2016

Uh oh!

bors commented Oct 17, 2016

Uh oh!

bors commented Oct 17, 2016

Uh oh!

bors commented Oct 17, 2016

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakisOct 17, 2016

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Oct 17, 2016

Uh oh!

bors commented Oct 17, 2016

Uh oh!

nikomatsakis commented Oct 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pnkfelix commented Oct 12, 2016•
edited by arielb1
Loading

pnkfelix commented Oct 12, 2016•
edited
Loading

arielb1 commented Oct 17, 2016•
edited
Loading